-
Notifications
You must be signed in to change notification settings - Fork 455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[query] Batch remote server responses #1784
Conversation
|
||
err = stream.Send(response) | ||
if err != nil { | ||
logger.Error("unable to send fetch result", zap.Error(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will want to return err
here if send fails, it's quite likely the connection is broken and should just terminate the send.
logger.Error("unable to send search result", zap.Error(err)) | ||
err = stream.Send(response) | ||
if err != nil { | ||
logger.Error("unable to send search result", zap.Error(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, should return err
here if send fails, it's quite likely the connection is broken and should just terminate the send.
src/query/tsdb/remote/server.go
Outdated
return err | ||
} | ||
|
||
err = stream.SendMsg(response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, think you want the type-safe stream.Send(response)
here.
|
||
err = stream.SendMsg(response) | ||
if err != nil { | ||
logger.Error("unable to send complete tags result", zap.Error(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, should return err
here if send fails, it's quite likely the connection is broken and should just terminate the send.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once rebased, you'll want to put the fetchOpts.Remote = true
in decodeFetchOptions
inside the remote
package.
# Conflicts: # src/query/tsdb/remote/server.go
Codecov Report
@@ Coverage Diff @@
## master #1784 +/- ##
========================================
- Coverage 72% 70.3% -1.8%
========================================
Files 986 977 -9
Lines 82958 82802 -156
========================================
- Hits 59807 58260 -1547
- Misses 19211 20527 +1316
- Partials 3940 4015 +75
Continue to review full report at Codecov.
|
src/query/storage/m3/storage.go
Outdated
@@ -380,6 +383,33 @@ func (s *m3storage) CompleteTags( | |||
wg sync.WaitGroup | |||
) | |||
|
|||
if err != nil { | |||
return nil, err | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm think the error above is already checked, maybe this was a side effect of a merge?
What this PR does / why we need it:
Fixes #1730
Does this PR introduce a user-facing and/or backwards incompatible change?: