Push NATS error messages to error queue #724

Amusement · 2018-06-05T18:36:52Z

Addressed comments -- removed max retries cap, improved error handling.

This change is

soamvasani

Looks great! Some very minor comments below, but other than that good to go 👍

soamvasani · 2018-06-08T00:01:04Z

mqtrigger/messageQueue/nats.go

-		if err != nil {
-			log.Warningf("Request failed: %v", url)
+		var resp *http.Response
+		for attempt := -1; attempt < trigger.Spec.MaxRetries; attempt++ {


This is entirely subjective but I'd have this loop go attempt := 0; attempt <= trigger.Spec.MaxRetries; ...

soamvasani · 2018-06-08T00:01:16Z

mqtrigger/messageQueue/nats.go

+		var resp *http.Response
+		for attempt := -1; attempt < trigger.Spec.MaxRetries; attempt++ {
+			// Make the request
+			log.Warningf("Request : %v", req)


soamvasani · 2018-06-08T00:03:07Z

mqtrigger/messageQueue/nats.go

+		}
+
+		if resp == nil {
+			log.Warning("The response was undefined. Quit.")


This means every retry failed, right? That's probably a better log message.

soamvasani · 2018-06-08T00:05:46Z

mqtrigger/messageQueue/nats.go

 			return
 		}
+
+		// TODO : Sequence


soamvasani · 2018-06-08T00:09:32Z

mqtrigger/messageQueue/nats.go

-			log.Printf("Request returned failure: %v", resp.StatusCode)
-			return
+
+		// TODO : what if err is not nil, then body is empty. publish either of them, not just body.


We shouldn't mix those errors into the same queue, simply avoiding publishing when body is empty is probably good enough.

This comment was actually added by @smruthi2187. I can remove this TODO and check if the body is empty before publishing.

soamvasani · 2018-06-08T00:10:54Z

mqtrigger/messageQueue/nats.go

+
+		// Only the latest error response will be published to error topic
+		if err != nil || resp.StatusCode != 200 {
+			if len(trigger.Spec.ErrorTopic) > 0 {


&& len(body) > 0

life1347 · 2018-06-10T15:32:26Z

Reviewed 4 of 6 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

fission/main.go, line 197 at r2 (raw file):

	mqtRespTopicFlag := cli.StringFlag{Name: "resptopic", Usage: "Topic that the function response is sent on (optional; response discarded if unspecified)"}
	mqtErrorTopicFlag := cli.StringFlag{Name: "errortopic", Usage: "Topic that the function error messages are sent to (optional; errors discarded if unspecified"}
	mqtMaxRetries := cli.IntFlag{Name: "maxretries", Usage: "Maximum number of times the function will be retried upon failure (optional; default is 0)"}

Use "Value" to set the default value 0 like

targetcpu := cli.IntFlag{Name: "targetcpu", Value: 80, Usage: "Target average CPU usage percentage across pods for scaling"}

fission/mqtrigger.go, line 163 at r2 (raw file):

	}
	// Default number of max retries is 0
	mqt.Spec.MaxRetries = 0

This line should be removed or it will overwrite previously maxRetries setting if maxRetries has been set.

mqtrigger/messageQueue/nats.go, line 133 at r2 (raw file):

			resp, err = http.DefaultClient.Do(req)
			if err != nil || resp == nil {
				// Retry without referencing status code of nil response on the next line

Print error message if err is not nil

mqtrigger/messageQueue/nats.go, line 139 at r2 (raw file):

				// Success, quit retrying
				break
			}

Suggest using HTTP status const for the status code. (https://golang.org/pkg/net/http/#pkg-constants).

Comments from Reviewable

life1347 · 2018-06-12T09:53:43Z

Review status: 3 of 6 files reviewed, 11 unresolved discussions (waiting on @life1347, @soamvasani, and @Amusement)

mqtrigger/messageQueue/nats.go, line 133 at r3 (raw file):

			resp, err = http.DefaultClient.Do(req)
			if err != nil {
				log.Error(err)

We also need to print some useful information for debugging, how about

log.Errorf("Error invoking function for trigger %v: %v", trigger.Metadata.Name, err)

Comments from Reviewable

Amusement · 2018-06-22T17:16:51Z

Review status: 2 of 6 files reviewed, 11 unresolved discussions (waiting on @life1347, @soamvasani, and @Amusement)

fission/mqtrigger.go, line 163 at r2 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

This line should be removed or it will overwrite previously maxRetries setting if maxRetries has been set.

Done.

mqtrigger/messageQueue/nats.go, line 129 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

This is entirely subjective but I'd have this loop go attempt := 0; attempt <= trigger.Spec.MaxRetries; ...

Done.

mqtrigger/messageQueue/nats.go, line 131 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

remove

Done.

mqtrigger/messageQueue/nats.go, line 144 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

This means every retry failed, right? That's probably a better log message.

Done.

mqtrigger/messageQueue/nats.go, line 157 at r1 (raw file):

Previously, Amusement (Nafisa Shazia) wrote…

This comment was actually added by @smruthi2187. I can remove this TODO and check if the body is empty before publishing.

Done.

mqtrigger/messageQueue/nats.go, line 161 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

&& len(body) > 0

Done.

mqtrigger/messageQueue/nats.go, line 133 at r2 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

Print error message if err is not nil

Done.

mqtrigger/messageQueue/nats.go, line 139 at r2 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

Suggest using HTTP status const for the status code. (https://golang.org/pkg/net/http/#pkg-constants).

Done.

mqtrigger/messageQueue/nats.go, line 133 at r3 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

We need to also print some useful information for debugging, how about
log.Errorf("Error invoking function for trigger %v: %v", trigger.Metadata.Name, err)

Done.

Comments from Reviewable

Amusement · 2018-06-22T17:17:39Z

Review status: 2 of 6 files reviewed, 11 unresolved discussions (waiting on @life1347 and @soamvasani)

fission/main.go, line 197 at r2 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

Use "Value" to set the default value 0 like

targetcpu := cli.IntFlag{Name: "targetcpu", Value: 80, Usage: "Target average CPU usage percentage across pods for scaling"}

Done.

mqtrigger/messageQueue/nats.go, line 148 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Hm?

Done.

Comments from Reviewable

life1347 · 2018-06-26T14:33:23Z

LGTM

life1347 self-requested a review June 7, 2018 05:47

soamvasani reviewed Jun 8, 2018

View reviewed changes

smruthi2187 added this to the 0.9.0 milestone Jun 14, 2018

Amusement force-pushed the nats-errors branch from 0c2dbd0 to eafd148 Compare June 20, 2018 21:29

Amusement added 10 commits June 26, 2018 22:50

Added error queue

12f4947

Run build job

294324a

Formatting

bcc2cf2

Removed max retry cap, cleaned up

75ab5d6

Reverted travis.yml

a29dd8b

Update comments with changes

e583e88

Refining

06ad88b

Address comments

09f0128

Detailed error message

8638c00

Fixed fatal

d943821

life1347 force-pushed the nats-errors branch from 8baf8d8 to d943821 Compare June 26, 2018 14:51

life1347 merged commit 1ed59f0 into master Jun 26, 2018

life1347 deleted the nats-errors branch June 26, 2018 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push NATS error messages to error queue #724

Push NATS error messages to error queue #724

Amusement commented Jun 5, 2018 •

edited by soamvasani

Loading

soamvasani left a comment

soamvasani Jun 8, 2018

soamvasani Jun 8, 2018

soamvasani Jun 8, 2018

soamvasani Jun 8, 2018

soamvasani Jun 8, 2018

Amusement Jun 8, 2018 •

edited

Loading

soamvasani Jun 8, 2018

life1347 commented Jun 10, 2018

life1347 commented Jun 12, 2018 •

edited

Loading

Amusement commented Jun 22, 2018

Amusement commented Jun 22, 2018

life1347 commented Jun 26, 2018

Push NATS error messages to error queue #724

Push NATS error messages to error queue #724

Conversation

Amusement commented Jun 5, 2018 • edited by soamvasani Loading

soamvasani left a comment

Choose a reason for hiding this comment

soamvasani Jun 8, 2018

Choose a reason for hiding this comment

soamvasani Jun 8, 2018

Choose a reason for hiding this comment

soamvasani Jun 8, 2018

Choose a reason for hiding this comment

soamvasani Jun 8, 2018

Choose a reason for hiding this comment

soamvasani Jun 8, 2018

Choose a reason for hiding this comment

Amusement Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

soamvasani Jun 8, 2018

Choose a reason for hiding this comment

life1347 commented Jun 10, 2018

life1347 commented Jun 12, 2018 • edited Loading

Amusement commented Jun 22, 2018

Amusement commented Jun 22, 2018

life1347 commented Jun 26, 2018

Amusement commented Jun 5, 2018 •

edited by soamvasani

Loading

Amusement Jun 8, 2018 •

edited

Loading

life1347 commented Jun 12, 2018 •

edited

Loading