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

AKCORE-167-2: Minor bugfix, shareStateMap was being updated twice for a single WriteShareGroupState operation #1293

Open
wants to merge 2 commits into
base: kip-932
Choose a base branch
from

Conversation

chirag-wadhwa5
Copy link
Member

According to the CoordinatorRuntime code, the replay method is called eventually, which should be responsible for the updations to the TimelineHashMaps (shareStateMap in our case). The replay function correctly updates the shareStateMap with the generated records, but an additional update to the shareStateMap was being made in the writeState function to update the snapshot epoch, causing double updates for a single WriteShareGroupState operation. This PR removes this additional update in the writeState function and generates the corrected records with the correct snapshot epoch value, which will be put in the shareStateMap during replay

@chirag-wadhwa5 chirag-wadhwa5 requested review from a team as code owners June 13, 2024 06:18
Copy link
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

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

Please could you also improve the javadoc for the readState and writeState methods. The comments say what the current limitations are, but a sentence at the start to say what the method is for would be appropriate. Thanks.

@@ -251,20 +251,18 @@ public CoordinatorResult<WriteShareGroupStateResponseData, Record> writeState(Re
WriteShareGroupStateResponseData responseData = new WriteShareGroupStateResponseData();
for (Record record : recordList) { // should be single record
if (record.key().message() instanceof ShareSnapshotKey && record.value().message() instanceof ShareSnapshotValue) {
ShareSnapshotKey newKey = (ShareSnapshotKey) record.key().message();
ShareSnapshotValue newValue = (ShareSnapshotValue) record.value().message();
ShareSnapshotKey recordKeyKey = (ShareSnapshotKey) record.key().message();
Copy link
Member

Choose a reason for hiding this comment

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

recordKeyKey??? I know it's trivial but please can we have a less comical name :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review, sorry about this 😅 The changes are there in the latest commit

@AndrewJSchofield AndrewJSchofield removed the request for review from a team June 13, 2024 07:31
@chirag-wadhwa5 chirag-wadhwa5 requested a review from smjn June 18, 2024 14:00
Copy link
Member

@smjn smjn left a comment

Choose a reason for hiding this comment

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

LGTM

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

3 participants