ctubbsii commented on code in PR #5192:
URL: https://github.com/apache/accumulo/pull/5192#discussion_r1888906333
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -126,9 +126,7 @@ public class ClientContext implements AccumuloClient {
private static final Logger log =
LoggerFactory.getLogger(ClientContext.class);
private final ClientInfo info;
- private InstanceId instanceId;
- private final ZooReader zooReader;
Review Comment:
One of the goals was to detangle the interweaving of a bunch of things that
made it hard to reason about the lifecycle of things. In a sense, ZooReader and
ZooReaderWriter were demoted from first-class citizens in the internals to mere
decorators/facade that lightly wrap the actual business object, the ZooKeeper.
Previously, one would get the ZooReader, which internally would create a
ZooKeeper for its business, using ZooSession which had global state and an
object pool. It was all very complex. Now, the business object is ZooKeeper.
You have one per context. If you want to decorate it with a ZooReader or
ZooReaderWriter, you can, but it's completely optional, and has no impact on
the lifecycle of the ZooKeeper object. It is much more clear this way.
One of the things I saw in the code was the construction of a ZooReader
object merely to carry around the ZooKeeper, but it was never actually used as
a ZooReader. This was the case in ZooCache, for example. It was just being used
a proxy to carry around a mechanism to construct a ZooKeeper using ZooSession,
which was all done internal to ZooReader. What ZooCache actually needs is just
a straight up ZooKeeper object... nothing more. So now, it gets that. If it
wants to use ZooReader or ZooReaderWriter, it can, but when they go away, the
ZooKeeper used by it doesn't, because they are just facades.
It is quite common to use the convenience methods provided by
ZooReader(Writer), so perhaps it is good to still have one on the Contexts.
However, by keeping them in the Context objects, I worry that people will
continue treating them as more than the facade that they are. But, now they are
very lightweight and stateless, so there's not really much harm in creating
one, as needed, instead, and just getting the ZooKeeper from the Context.
One reason why ZooReader was being passed around is because it has a
`getZooKeeper()` method to get the ZooKeeper object it wraps. For that reason,
we had a ton of internal APIs passing around a ZooReader object, but never
actually using it, and then eventually the ZooKeeper was pulled out of it, and
the ZooReader object didn't matter. That is very confusing to understand when
going through so many layers. In the past, that was the best way to get a
ZooKeeper, though. But now, you just get that directly from the Context. So,
instead of needing to pass around the context everywhere, plus a ZooReader, you
just pass around the context and grab the ZK when needed, and then wrap it if
needed. I would actually like to remove the `getZooKeeper()` method entirely
from ZooReader to prevent its abuse as a carrier for ZooKeeper, but there is
one part of the code in `ZooPropStore` that uses the session timeout dangling
off the ZooKeeper inside the ZooReader for the default implementation of a
n object called a ReadyMonitor, and it's a bit unnecessarily complex... but
hard to figure out how to simplify it (actually, that part of the code is an
example of what I'm talking about... the method takes an InstanceId and a
ZooReaderWriter, but it should have just accepted a single ServerContext
instead, since that's the object that carries the client or service's session
state, and we don't need to partially split it into two params when one would
suffice). So long as that ZooPropStore stuff exists, I have to keep
`getZooKeeper()` and that makes me more reluctant to keep a ZooReader(Writer)
on the context, because it's more prone to abuse. But, I'm also just generally
reluctant, because I don't want ZooReader(Writer) to become first-class
citizens in the ZooKeeper management code again... it sets us up for confusion
and failure when we try to chroot it and reason about which ZooKeeper it is
wrapping, and whether it's a chrooted ZooKeeper or a non-chrooted one.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
