HBASE-29313 RecoverableZooKeeper.getZooKeeper() returns null for new object #6987
Conversation
stoty
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I'm not sure whether we should initialize zookeeper when calling getZooKeeper, as returning null also indicate that there is no request yet, and can be used to as a test condition.
But anyway, since this class is IA.Private, we are OK to change the behavior if it does not break anything, just more javadoc to explain the logic.
| // Callers expect an initialized ZooKeeper instance | ||
| // Pre HBASE-28529 the constructor used to call checkZk() | ||
| try { | ||
| checkZk(); |
| try { | ||
| checkZk(); | ||
| } catch (Exception x) { | ||
| // Just return null zk |
There was a problem hiding this comment.
Better log the exception and return null?
| @@ -726,6 +726,13 @@ public synchronized States getState() { | |||
| } | |||
|
|
|||
| public synchronized ZooKeeper getZooKeeper() { | |||
There was a problem hiding this comment.
checkZk itself is synchronized, so we can remove the synchronized?
There was a problem hiding this comment.
We probably could, but I don't think this is a performance hotspot, so I'd just leave it as is.
None of the existing code uses it like that. I think that ideally getZokkeper() would throw an exception just like most other RecoverableZooKeeper methods, but the callers aren't expecting that, and I didn't want to makemore changes to handle re-tries etc in all of the callers. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…object (#6987) Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit d9b1aa1)
…object