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: migration ACK response processing #4140

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

BorysTheDev
Copy link
Contributor

problem: the migration process can get stuck because of the pause between migration finalization attempts for #4081

fix: try reading the correct ACK response several times until timeout happens

Copy link
Collaborator

@chakaz chakaz left a comment

Choose a reason for hiding this comment

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

I don't quite understand this fix.
With the fix, will attempt to read multiple times. But will we ever send multiple ACKs? I get that if it times out, we may want to try and read again (why not wait longer though?), but in which circumstance will we see more than 1 ACK? If previous ACKs are waiting? If that's the case, there could be other data waiting in line, no? (like regular journal)

@BorysTheDev
Copy link
Contributor Author

@chakaz When get time out we don't know if the data is lost or delayed we try to resend ack but can get the previous response in this case we try to read one more time. In the main connection can't be other data, All other data is sent via flow connections

Copy link
Collaborator

@chakaz chakaz left a comment

Choose a reason for hiding this comment

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

Makes sense!

@BorysTheDev BorysTheDev merged commit 5e2b48c into main Nov 18, 2024
12 checks passed
@BorysTheDev BorysTheDev deleted the fix_MIGRATION_ACK_RESPONSE_READING branch November 18, 2024 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants