Skip to content

Remove TODOs from RootTabletMutatorImpl.java - #6494

Merged
dlmarion merged 6 commits into
apache:mainfrom
Amemeda:mutator-todos
Aug 11, 2026
Merged

Remove TODOs from RootTabletMutatorImpl.java#6494
dlmarion merged 6 commits into
apache:mainfrom
Amemeda:mutator-todos

Conversation

@Amemeda

@Amemeda Amemeda commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Removed the following TODOs:

  • RootTabletMutatorImpl.java: // TODO examine implementation of getZooReaderWriter().mutate(): This TODO may be outdated, current version of code does not use context.getZooReaderWriter(), uses context.getZooSession() instead
  • RootTabletMutatorImpl.java: // TODO for efficiency this should maybe call mutateExisting: Updated line 103 to use mutateExisting() instead of mutateOrCreate()
  • RootTabletMutatorImpl.java: // TODO this is racy... . Removed 1 usage of getZooCache().clear(). Since Zoocache already invalidates data when ZooKeeper reports a data change, this call is redundant and racy.
  • RootConditionalWriter.java: // TODO this is racy... . Same as above
image

This pr resolves 4 TODOs from #2699

@Amemeda Amemeda changed the title Remove TODO's from RootTabletMutatorImpl.java Remove TODOs from RootTabletMutatorImpl.java Jul 29, 2026
@Amemeda
Amemeda marked this pull request as ready for review August 4, 2026 17:14

@DomGarguilo DomGarguilo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is the right approach. I think expexting .get() to return a length of 0 wont happen because .clear() clears the value from the cache while .get() pulls the actual data from ZooKeeper which is not removed by .clear().

I think the correct approach here is actually just drop the 3 .clear calls (there are two in RootConditionalWriter.java) since ZooCache already invalidates things when ZooKeeper reports a data change. So .clear() is redundant and racy.

For the TODOs in RootTabletMutatorImpl.java, I think the proper fix is to just drop the .clear() call for the same reasons. And the other TODO can be fixed by simply using mustateExisting(). I think thats the correct approach because we are expecting things to already exist and using mutateExisting() will fail if it doesn't exist rather than create things in a bad state.

@Amemeda
Amemeda requested a review from DomGarguilo August 5, 2026 14:38

@DomGarguilo DomGarguilo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As far as i can tell things are correct here. It would be good if others could verify things too before this is merged.

@ctubbsii ctubbsii added this to the 4.0.0 milestone Aug 6, 2026

@ctubbsii ctubbsii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The change to mutateExisting is fine, because the node is created during initialization or ugprade, so it should be there. Clearing the ZooCache for the root tablet shouldn't affect the mutateExisting behaviors, since that code doesn't use ZooCache, but I'm not really sure why it was there in the first place. It's possible we're clearing because other code that does use ZooCache won't necessarily get the updated data right away. The comment about it being "racy" didn't really elaborate. However, it's probably okay to leave the cache cleared after the mutateExisting in RootConditionalWriter.

@Amemeda
Amemeda requested a review from dlmarion August 7, 2026 17:43
@dlmarion
dlmarion merged commit 6ce09f9 into apache:main Aug 11, 2026
8 checks passed
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.

4 participants