[GitHub] [accumulo] keith-turner commented on issue #1086: ZooLock uses ZooReader which may retry on session expired
keith-turner commented 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 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: us...@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #1086: ZooLock uses ZooReader which may retry on session expired
keith-turner commented on issue #1086: URL: https://github.com/apache/accumulo/issues/1086#issuecomment-668082852 @milleruntime I suspect its still around. I will take a look and post some code links if I find its still an issue. 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: us...@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #1086: ZooLock uses ZooReader which may retry on session expired
keith-turner commented on issue #1086: ZooLock uses ZooReader which may retry on session expired URL: https://github.com/apache/accumulo/issues/1086#issuecomment-552017270 I looked into this a bit today. Another issue I discovered with ZooLock's use of ZooReader is that it automatically creates new Zookeeper objects when a session has expired. I am not sure if this is causing any problems, but its a concern I have. The following is some background info. * Zookeeper objects have a one to relationship with a zookeeper session. When a session expires zookeeper objects become non-functional. * Zoolock relies on ephemeral nodes which are automatically deleted when a session expires. It may be ok that new Zookeeper objects are automatically created because the ephemeral node will cease to exist and this would be visible through the new zookeeper object. Because of the tight coupling between zoolock, ephemeral nodes, zookeeper objects, and zookeeper sessions I think it may be better if zoolock directly uses zookeeper w/o any intermediate abstraction layer. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [accumulo] keith-turner commented on issue #1086: ZooLock uses ZooReader which may retry on session expired
keith-turner commented on issue #1086: ZooLock uses ZooReader which may retry on session expired URL: https://github.com/apache/accumulo/issues/1086#issuecomment-519970692 I added this a blocker because I think it would be good to investigate before 1.9.4. Since its a long standing issue not introduced in 1.9.3, It need not block 1.9.4 if not investigated in time. 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: us...@infra.apache.org With regards, Apache Git Services
