Remove TODOs from RootTabletMutatorImpl.java

[Amemeda]

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

This pr resolves 4 TODOs from #2699

[DomGarguilo]

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]

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]

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.

[ctubbsii]

I think other code may benefit from having this stay here. I don't really know what the comment refers to or whether it matters.

[ctubbsii]

@dlmarion has previously worked on the ZooCache stuff and might have more insight into whether these cache evictions are useful.

[dlmarion]

The code above just mutated the root tablet metadata in ZooKeeper. IIRC, the ZCacheWatcher will fire at some point in the future and receive a NodeDataChanged event, and it will clear the cache using this same type of call. However, until that Watcher fires, ZooCache will contain and return the old stale data.

I think the comment here about this being a race condition was suggesting that the clear may/will happen twice potentially in quick succession. However, I think it's still required here because we don't know when, the Watcher will fire.

I think this clear needs to stay.

[ctubbsii]

@dlmarion I'll believe you if you say yes, but I was thinking we probably don't need the clear before the update, since we're not reading from ZooCache to perform the update, and there's no point in clearing it until after we're done making the changes. Are you sure we need the one before hand?

[dlmarion]

The code above just mutated the root tablet metadata in ZooKeeper. IIRC, the ZCacheWatcher will fire at some point in the future and receive a NodeDataChanged event, and it will clear the cache using this same type of call. However, until that Watcher fires, ZooCache will contain and return the old stale data.

I think the comment here about this being a race condition was suggesting that the clear may/will happen twice potentially in quick succession. However, I think it's still required here because we don't know when, the Watcher will fire.

I think this clear needs to stay.