Skip to content
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

fix issue 650,different handle sequence value #651

Merged
merged 2 commits into from Nov 4, 2021

Conversation

baomingyu
Copy link
Contributor

Fixes #650

Modify handle CommandSendReceipt command way same with java client ,
and when response's sequenceID is less than max sequenceID in pendingQueue will not close cnx.

Master Issue: #650

@baomingyu
Copy link
Contributor Author

@merlimat @freeznet @wolfstudy PTAL

@@ -784,57 +784,57 @@ func (p *partitionProducer) ReceivedSendReceipt(response *pb.CommandSendReceipt)
return
}

if pi.sequenceID != response.GetSequenceId() {
if pi.sequenceID < response.GetSequenceId() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the response sequence id is greater than the pi.sequenceID? Is that case handled anywhere and does it need to be? How does this change reduce the number of closed connections?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

response sequence id greater than the pi.sequenceID is error case, response sequence id must be equal or less than pi.sequenceID in one connection

Copy link
Contributor

@cckellogg cckellogg Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that. The previous code would return an error and close the connection when the sequenceId did not match. So the sequence id could be greater or less than. Unless I'm missing something this change seems to only return an error when the sequenceId is less.

Does the client need to handle the case when the sequence id is greater? Can that happen?

Also, for my own understanding does this change reduce the number of reconnections mentioned in the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change will be reduce the number of reconnections only. this is very important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now there are 2000 pod to consume one topic, mean there are 2000 consumers for one partitions, if this code is not bean changed , many times consumers need to be restarted.
Another words , this handle way is different with java sdk.

Copy link
Member

@wolfstudy wolfstudy Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cckellogg It seems that in some scenarios, the condition of pi.sequenceID <response.GetSequenceId() will be true, causing the connection to be closed. And @baomingyu they have more consumer objects under one connection, so when the reconnection happens, the impact on their business itself is magnified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pi.sequenceID < response.GetSequenceId() is error case,the channel must be closed when it has happed. And
pi.sequenceID > response.GetSequenceId() case should not do close way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are you suggesting we follow the logic of the java client and handle the 3 cases this way?

1 - pi.sequenceID > response.GetSequenceId() => return (could there ever be a pending item in the queue that gets left around?)

2 - pi.sequenceID < response.GetSequenceId() => error close connection

3 - pi.sequenceID == response.GetSequenceId() => process

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, @cckellogg
Client should reduce close connection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a log for the last condition pi.sequenceID > response.GetSequenceId() just to track if we ever see it?

Also, if this condition ever happens do we leak pending items?

@wolfstudy wolfstudy added this to the v0.8.0 milestone Nov 1, 2021
@baomingyu baomingyu removed their assignment Nov 1, 2021
Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1

@wolfstudy wolfstudy merged commit d80a722 into apache:master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants