Sending live loader error output to Stderr #3560
Conversation
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @manishrjain)
There was a problem hiding this comment.
Should these error logs go in a more verbose log level? Knowing which particular txn got aborted/retried is useful for debugging, but not in general when all you want to do is load data. These abort logs overwhelm the typical 5-second log informational log.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gitlw and @manishrjain)
dgraph/cmd/live/batch.go, line 125 at r1 (raw file):
time.Sleep(dur) case err != y.ErrConflict: errLog.Printf("Error while mutating: %v\n", s.Message())
When running live loader this error message looks like this:
Error while mutating: Transaction has been aborted. Please retry.
Showing "Please retry" for live loading isn't helpful. Live loader will retry already on behalf of the user.
This particular error message isn't technically an "error" with live loader anyway. This should be handled by live loader.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)
dgraph/cmd/live/batch.go, line 125 at r1 (raw file):
Previously, danielmai (Daniel Mai) wrote…
When running live loader this error message looks like this:
Error while mutating: Transaction has been aborted. Please retry.Showing "Please retry" for live loading isn't helpful. Live loader will retry already on behalf of the user.
This particular error message isn't technically an "error" with live loader anyway. This should be handled by live loader.
I agree. So how about we add a -v flag to indicate whether these log lines should be shown?
|
dgraph/cmd/live/batch.go, line 125 at r1 (raw file): Previously, gitlw (Lucas Wang) wrote…
Live loader already has |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)
dgraph/cmd/live/batch.go, line 125 at r1 (raw file):
This should be handled by live loader.
What I meant by the above is that we don't need to print out transaction abort errors since they can happen with live loader and concurrent mutations. Live loader will retry and it's ok. The user will see the "Aborts:" counter go up, and no action by them is needed for temporary aborts. However, there can be other errors during mutations like invalid input formats that the user should know about, meaning this shouldn't be behind the verbose flag.
In v1.0.15 live loader handles the y.ErrAborted case. We can revert this line from this change to bring back this behavior: f314b3f#diff-b6f7fdc77028b083a4d14d4fb14aa431L139.
dgraph/cmd/live/batch.go, line 119 at r2 (raw file):
dur := time.Duration(1+rand.Intn(10)) * time.Minute if opt.verbose { fmt.Printf("Server is overloaded. Will retry after %s.\n", dur.Round(time.Minute))
The "transaction abort" logs can be behind the verbose flag. The server being overloaded would be important for the user to know about and shouldn't be part of verbose.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @danielmai, @gitlw, and @manishrjain)
dgraph/cmd/live/batch.go, line 125 at r1 (raw file):
Previously, danielmai (Daniel Mai) wrote…
This should be handled by live loader.
What I meant by the above is that we don't need to print out transaction abort errors since they can happen with live loader and concurrent mutations. Live loader will retry and it's ok. The user will see the "Aborts:" counter go up, and no action by them is needed for temporary aborts. However, there can be other errors during mutations like invalid input formats that the user should know about, meaning this shouldn't be behind the verbose flag.
In v1.0.15 live loader handles the
y.ErrAbortedcase. We can revert this line from this change to bring back this behavior: f314b3f#diff-b6f7fdc77028b083a4d14d4fb14aa431L139.
Ok, I changed it so that the "server overloaded" and other types of errors would still be shown even without the verbose flag.
Regarding the y.ErrAborted case, that PR added the "case s.Code() == codes.Aborted:" check, which should handle the aborted txns, so I think the current logic looks fine.
There was a problem hiding this comment.
Given these latest changes, does this PR still intend to output to stderr? Or was the main goal to surpress some logs, in which case the log verbosity flag does that.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gitlw and @manishrjain)
dgraph/cmd/live/batch.go, line 125 at r1 (raw file):
Regarding the y.ErrAborted case, that PR added the "case s.Code() == codes.Aborted:" check, which should handle the aborted txns, so I think the current logic looks fine.
It doesn't handle aborted txns. I see Error while mutating: Transaction has been aborted. Please retry. when I run live loader today. Do you see the same?
AFAIK codes.Aborted is for gRPC-aborted messages, not Dgraph-txn-aborted messages.
|
Is this being worked on? Or, can be closed? |
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)
dgraph/cmd/live/batch.go, line 125 at r1 (raw file):
Previously, danielmai (Daniel Mai) wrote…
Regarding the y.ErrAborted case, that PR added the "case s.Code() == codes.Aborted:" check, which should handle the aborted txns, so I think the current logic looks fine.
It doesn't handle aborted txns. I see
Error while mutating: Transaction has been aborted. Please retry.when I run live loader today. Do you see the same?AFAIK
codes.Abortedis for gRPC-aborted messages, not Dgraph-txn-aborted messages.
You are right, I've added checking for ErrAborted and reverted to the initial behavior, which is not logging for ErrAborted.
There was a problem hiding this comment.
@danielmai The purpose is to suppress the excessive logs. With the verbose flag, there is no need to redirect to stderr anymore.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)
This change is