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.
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.... AddedThread.sleep()after the first call to clear() as a makeshift wait to check that the ZROOT_TABLET is empty before calling clear() again a few lines later to avoid race condition.RootConditionalWriter.java:// TODO this is racy.... Same as aboveThis pr resolves 4 TODOs from #2699
Need more information on what is going on here / what an alternative would be. Both occurrences of this TODO are preceded by the exact same clear() a few lines beforehand, could this be why it is racy?