Implemented Feature which enable Connector to leverage Responses #4

Templum · 2019-01-20T16:26:55Z

Addresses Feature wishes from #2

Changes:

Invoke Signature changed from func (i *Invoker) Invoke(topicMap *TopicMap, topic string, message *[]byte) => func (i *Invoker) Invoke(topicMap *TopicMap, topic string, message *[]byte) (result map[string]*InvocationResult)
Renamed invokefunction to performInvocation and changed signature to performInvocation(c *http.Client, gwURL string, reader io.Reader) (err error, statusCode int, responseHeaders http.Header, responseBody *[]byte
If PrintResponse is true it will now also log the headers
Added Tests for Invoker covering some cases. Further logs show also the printing of header

Signed-off-by: Simon Pelczer [email protected]

Signed-off-by: Simon Pelczer <[email protected]>

embano1 · 2019-01-24T19:18:15Z

types/invoker_test.go

+		results := target.Invoke(&topicMap, "Success", &body)
+
+		if len(results) != 0 {
+			t.Errorf("Expected 0 results recieved %d", len(results))


Typo "recieved" -> "received"

embano1 · 2019-01-24T19:19:37Z

types/invoker.go

 }

-func invokefunction(c *http.Client, gwURL string, reader io.Reader) (*[]byte, int, error) {
+func performInvocation(c *http.Client, gwURL string, reader io.Reader) (err error, statusCode int, responseHeaders http.Header, responseBody *[]byte) {


Would it make sense to also return a struct (like InvokationResult above) instead of four individual response items?

embano1 · 2019-01-24T19:24:33Z

types/invoker.go


-			if doErr != nil {
-				log.Printf("Unable to invoke from %s, error: %s\n", matchedFunction, doErr)
+			if err != nil {


Looks like we're returning here too soon, i.e. if there's multiple matchedFuntions, others would not be executed if an error is thrown early. There could be an error with the receiving function (bug), others could be fine.

embano1 · 2019-01-24T19:25:01Z

types/invoker.go

 }

-func invokefunction(c *http.Client, gwURL string, reader io.Reader) (*[]byte, int, error) {
+func performInvocation(c *http.Client, gwURL string, reader io.Reader) (err error, statusCode int, responseHeaders http.Header, responseBody *[]byte) {


Consider making this a method on the invoker to avoid passing around objects multiple times.

embano1 · 2019-01-24T19:25:18Z

types/invoker.go

 }

-func invokefunction(c *http.Client, gwURL string, reader io.Reader) (*[]byte, int, error) {
+func performInvocation(c *http.Client, gwURL string, reader io.Reader) (err error, statusCode int, responseHeaders http.Header, responseBody *[]byte) {

 	httpReq, _ := http.NewRequest(http.MethodPost, gwURL, reader)


No error check here?

embano1 · 2019-01-24T19:26:01Z

types/invoker.go

@@ -63,7 +73,7 @@ func invokefunction(c *http.Client, gwURL string, reader io.Reader) (*[]byte, in

 	res, doErr := c.Do(httpReq)
 	if doErr != nil {
-		return nil, http.StatusServiceUnavailable, doErr
+		return doErr, http.StatusServiceUnavailable, nil, nil


Should we distinct between HTTP error messages here, instead of always a "503"?

Templum · 2019-01-24T20:41:32Z

@embano1 Thanks for the input. I wanted originally to keep the same behaviour, however seeing you also raise those concerns convinced me to address them. Will push them in a new update.

* Using a Struct for return value for performInvocation * No longer aborting requests on first error * performInvocation is now method of Invoker * Handled NewRequest error, altough this only happens when miss configured Signed-off-by: Simon Pelczer <[email protected]>

* They now check that an failing function wont abort invocations * They check that no invocation is performed for invalid messages * They check error is only set when an error occurred Signed-off-by: Simon Pelczer <[email protected]>

alexellis · 2019-01-25T11:43:05Z

Hi both, please can we bear in mind any breaking changes for existing connectors (of which there are several, and not all are maintained by the project)

Alex

alexellis · 2019-01-25T11:43:31Z

types/invoker.go


-			log.Printf("Invoke function: %s", matchedFunction)
+type InvocationResponse struct {


InvocationResponse vs InvocationResult ?

alexellis · 2019-01-25T11:43:42Z

types/invoker.go


-		matchedFunctions := topicMap.Match(topic)
-		for _, matchedFunction := range matchedFunctions {
+func NewInvocationResult(statusCode int, body *[]byte, error error) *InvocationResult {


This looks redundant to me.

alexellis · 2019-01-25T11:43:58Z

types/invoker.go


-			gwURL := fmt.Sprintf("%s/function/%s", i.GatewayURL, matchedFunction)
-			reader := bytes.NewReader(*message)
+func NewInvocationResponse(statusCode int, body *[]byte, headers http.Header) *InvocationResponse {


Seems like it adds no value?

alexellis · 2019-01-25T11:44:42Z

types/invoker.go

 }

-func invokefunction(c *http.Client, gwURL string, reader io.Reader) (*[]byte, int, error) {
+func (i *Invoker) performInvocation(functionURL string, bodyReader io.Reader) (*InvocationResponse, error) {


Please revert this to the more natural sounding invokefunction, perhaps capitalize to invokeFunction or just invoke.

alexellis

I would prefer to see this PR put on hold and a new PR raised which simply does the one thing the original issued ask for without any additional refactoring.

The refactoring makes this hard for me to review and verify and is not necessary for the sake of the original requirement - return status, body and error instead of void.

ivanayov · 2019-01-25T12:01:13Z

@Templum I'd suggest the following:

Keep this, but raise a new PR as minimal as possible to cover the requirements in Update invokefunction to return headers #2
Open an issue for the refactoring work, stating why you find it useful and if it's approved, you can go on with the work already done or started.

alexellis · 2019-01-25T12:59:31Z

types/invoker_test.go

+)
+
+func TestInvoker_Invoke(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


If you'd like to back-port your tests to my PR that would be appreciated.

I've implemented the changes with a subscriber model that should make testing easier also.

Implemented Feature which enable Connector to leverage Responses

fc0c5c6

Signed-off-by: Simon Pelczer <[email protected]>

derek bot added the new-contributor label Jan 20, 2019

Templum mentioned this pull request Jan 24, 2019

Add retry/durability options if Invoke() fails #5

Closed

embano1 reviewed Jan 24, 2019

View reviewed changes

Templum added 2 commits January 24, 2019 23:03

Adjusted Tests

d8420ed

* They now check that an failing function wont abort invocations * They check that no invocation is performed for invalid messages * They check error is only set when an error occurred Signed-off-by: Simon Pelczer <[email protected]>

alexellis reviewed Jan 25, 2019

View reviewed changes

alexellis requested changes Jan 25, 2019

View reviewed changes

alexellis added the blocked/too-many-changes label Jan 25, 2019

alexellis mentioned this pull request Jan 25, 2019

Receive responses via subscribers #8

Merged

alexellis reviewed Jan 25, 2019

View reviewed changes

alexellis closed this in #8 Jan 25, 2019

Templum mentioned this pull request Jan 25, 2019

Ported Tests over #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Feature which enable Connector to leverage Responses #4

Implemented Feature which enable Connector to leverage Responses #4

Templum commented Jan 20, 2019 •

edited

Loading

embano1 Jan 24, 2019

embano1 Jan 24, 2019

embano1 Jan 24, 2019

embano1 Jan 24, 2019

embano1 Jan 24, 2019

embano1 Jan 24, 2019

Templum commented Jan 24, 2019

alexellis commented Jan 25, 2019

alexellis Jan 25, 2019

alexellis Jan 25, 2019

alexellis Jan 25, 2019

alexellis Jan 25, 2019

alexellis left a comment

ivanayov commented Jan 25, 2019

alexellis Jan 25, 2019


		log.Printf("Invoke function: %s", matchedFunction)
		type InvocationResponse struct {

Implemented Feature which enable Connector to leverage Responses #4

Implemented Feature which enable Connector to leverage Responses #4

Conversation

Templum commented Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Templum commented Jan 24, 2019

alexellis commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis left a comment

Choose a reason for hiding this comment

ivanayov commented Jan 25, 2019

Choose a reason for hiding this comment

Templum commented Jan 20, 2019 •

edited

Loading