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

Handle isolation level in Offset(Request|Response) and require stable offset in FetchOffset(Request|Response) #1929

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

celrenheit
Copy link
Contributor

This PR enables OffsetRequest and to require read committed isolation.
It adds RequireStable field to FetchOffsetRequest, this requires the use of the new compact protocol present in version 6+.

@celrenheit celrenheit requested a review from bai as a code owner April 29, 2021 13:29
@ghost ghost added the cla-needed label Apr 29, 2021
@celrenheit celrenheit force-pushed the fetch-offset-require-stable-offset branch from 1500bf1 to 3f98e25 Compare April 29, 2021 14:08
@ghost ghost removed the cla-needed label Apr 29, 2021
broker.go Show resolved Hide resolved
@celrenheit celrenheit force-pushed the fetch-offset-require-stable-offset branch 2 times, most recently from f4db789 to c5e3fdd Compare April 29, 2021 16:54
Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, I eyeballed the protocol changes against the protocol specs and the version matrix and the changes look good to me

broker.go Show resolved Hide resolved
It enables list available offset (OffsetRequest and OffsetResponse)
to require read committed isolation level.
It adds RequireStable field to FetchOffsetRequest.
@celrenheit celrenheit force-pushed the fetch-offset-require-stable-offset branch from c5e3fdd to fce917a Compare April 29, 2021 22:42
Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

Approved. Thanks again!

Note: the status checks may be a little flaky in the functional test atm and may need a re-run or two before we can merge

@celrenheit
Copy link
Contributor Author

I've rebased it on master so that it can pass the 2.8.0 test.
Thank you very much for reviewing this !

@dnwe dnwe merged commit 16dacfe into IBM:master Apr 29, 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants