Remove TODOs from RootTabletMutatorImpl.java - #6494
Conversation
DomGarguilo
left a comment
There was a problem hiding this comment.
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.
DomGarguilo
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
Removed the following TODOs:
RootTabletMutatorImpl.java:// TODO examine implementation of getZooReaderWriter().mutate(): This TODO may be outdated, current version of code does not usecontext.getZooReaderWriter(), usescontext.getZooSession()insteadRootTabletMutatorImpl.java:// TODO for efficiency this should maybe call mutateExisting: Updated line 103 to usemutateExisting()instead ofmutateOrCreate()RootTabletMutatorImpl.java:// TODO this is racy.... Removed 1 usage ofgetZooCache().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 aboveThis pr resolves 4 TODOs from #2699