[GitHub] [accumulo] keith-turner edited a comment on issue #1086: ZooLock uses ZooReader which may retry on session expired

[GitBox <gi...@apache.org>]

keith-turner edited a comment on issue #1086:
URL: https://github.com/apache/accumulo/issues/1086#issuecomment-670084908


   @milleruntime  I researched this a bit more, looking into the following two concerns.
   
    1. ZooReaderWriter retries on session expired exceptions.
    2. ZooReaderWriter automatically gets a new zookeeper when one is closed.
   
   For issue 1, looking at the code zoolock code it deals mainly deals with session expired in watcher.  As far as I can tell ZooReaderWriter does not alter events from watcher at the moment.  For other code in ZooLock, its difficult to analyze the impact of ZooReaderWriter retrying on session expired.  There were a few places I was concerned about that seemed like they may work by accident.  So for concern 1 I have yet to identify an actual bug through code analysis, but I suspect there could be potential bugs.
   
   For concern 2, I did find an actual race condition that seems like it could result in erroneous data being placed in the metadata table.
   
   In the code below, ZooLock calls ZooReaderWriter.getZooKeeper().getSessionId() to compute its LockId
   
   https://github.com/apache/accumulo/blob/rel/1.9.3/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java#L358-L363
   
   Below is the code for ZooReaderWriter.getZooKeeper().  If you follow this code you will see that it creates a new Zookeeper if the current one is closed.  For the above code that is problematic, because a new ZK object would have a different session id.
   
   https://github.com/apache/accumulo/blob/rel/1.9.3/fate/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java#L48-L54
   
   The code below is called by the tablet server and based on the previous code may put the wrong id in the metadata table.
   
   https://github.com/apache/accumulo/blob/rel/1.9.3/server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java#L135-L138
    
   For this particular case a fix may be to make ZooLock.getLockID call ZooLock.getSessionId() instead of ZooLock.zooKeeper.getZooKeeper().getSessionId().  But I am not completely sure.  Anyway this is one example of how the zookeeper silently switching under zoolock could be problematic, there may be other cases.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org