-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
store/tikv: coprocess streaming tiny fix #6186
Conversation
/run-all-tests |
/run-all-tests |
/run-all-tests |
/run-sqllogic-test |
/run-unit-test |
/run-unit-test |
1 similar comment
/run-unit-test |
/run-all-tests |
/run-integration-common-test |
/run-common-test |
1 similar comment
/run-common-test |
/run-all-tests |
/run-integration-common-test |
1 similar comment
/run-integration-common-test |
store/tikv/client.go
Outdated
if errors.Cause(err) != io.EOF { | ||
return nil, errors.Trace(err) | ||
} | ||
log.Info("copstream returns nothing for the request.") |
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.
Is it expected for tikv to close the stream directly?
If so, I think Debug
is better, otherwise I think Warn
is better.
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.
I'll change it to debug level
store/tikv/coprocessor.go
Outdated
@@ -652,7 +656,7 @@ func (it *copIterator) handleCopStreamResult(bo *Backoffer, stream *tikvrpc.CopS | |||
|
|||
// handleCopResponse checks coprocessor Response for region split and lock, | |||
// returns more tasks when that happens, or handles the response if no error. | |||
func (it *copIterator) handleCopResponse(bo *Backoffer, resp *coprocessor.Response, task *copTask, ch chan copResponse) ([]*copTask, error) { | |||
func (it *copIterator) handleCopResponse(bo *Backoffer, resp *coprocessor.Response, task *copTask, ch chan copResponse, lastResp *coprocessor.Response) ([]*copTask, error) { |
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.
Could you add the comment for lastResp
?
store/tikv/coprocessor.go
Outdated
func (it *copIterator) handleCopResponse(bo *Backoffer, resp *coprocessor.Response, task *copTask, ch chan copResponse) ([]*copTask, error) { | ||
// if we're handling streaming coprocessor response, lastResp is the last successful response, | ||
// if we're handling normal request, lastResp and resp is the same. | ||
func (it *copIterator) handleCopResponse(bo *Backoffer, resp *coprocessor.Response, task *copTask, ch chan copResponse, lastResp *coprocessor.Response) ([]*copTask, error) { |
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.
How about using lastRange *KeyRange
instead of lastResp *coprocessor.Response
?
It's easier to understand the purpose of the argument.
/run-all-tests |
LGTM |
PTAL @disksing |
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.
Tiny fix:
Recv()
is called.coprocessor.Response.Range
field when error happens.@coocood @winkyao @shenli