accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mike Drob" <md...@mdrob.com>
Subject Re: Review Request 16485: ACCUMULO-2103 - Better ZooSession management
Date Fri, 27 Dec 2013 21:44:56 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16485/#review30923
-----------------------------------------------------------



src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java
<https://reviews.apache.org/r/16485/#comment59154>

    Should, at a minimum, also set Thread.currentThread().interrupt() flag. Also, if this
FIXME persists, please create a JIRA for it and reference it here.



src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooKeeperFactory.java
<https://reviews.apache.org/r/16485/#comment59156>

    Should this be public?



src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooKeeperFactoryException.java
<https://reviews.apache.org/r/16485/#comment59155>

    I think Eclipse will complain about a SerialVersionUid.



src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooReader.java
<https://reviews.apache.org/r/16485/#comment59157>

    Synchronization issue? What happens when two threads attempt to access the first ZooKeeper
at the same time?



src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooSession.java
<https://reviews.apache.org/r/16485/#comment59158>

    Can this leak unclosed sessions?



src/core/src/test/java/org/apache/accumulo/core/zookeeper/ZooSessionTest.java
<https://reviews.apache.org/r/16485/#comment59159>

    Should verify that it didn't close after this.


- Mike Drob


On Dec. 27, 2013, 7:43 p.m., Bill Havanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16485/
> -----------------------------------------------------------
> 
> (Updated Dec. 27, 2013, 7:43 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-2103
>     https://issues.apache.org/jira/browse/ACCUMULO-2103
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> This is the work on ZooSession to better support closure of ZK connections under ACCUMULO-2026
and/or future work. Here's a tour.
> 
> First, the logic in ZooSession to connect to ZooKeeper was moved to a new ZooKeeperFactory
class. This also makes it possible to mock the logic for unit tests, or swap in newer / better
implementations later. The factory will throw a new ZooKeeperFactoryException (unchecked)
for most problems, but does throw InterruptedException when appropriate (instead of eating
it). Otherwise, the logic is the same.
> 
> Before this, ZooSession was a set of static methods oriented around connecting to ZooKeeper.
With this set of commits, ZooSession objects are used, each of which contains a ZooKeeper
connection object as well as its key (formed and used as before).
> 
> The ZooSession class still uses a static map of known sessions, so that the same connection
may be reused by several clients. The map now also remembers the watcher object associated
with the connection (possibly a prior oversight), and a reference count. Whenever a new connection
is established or it is reused, the reference count is increased. When a session is closed,
the reference count is decreased, and the connection itself is only closed when the reference
count is depleted. Cleanup of closed session objects occurs as it did before, lazily upon
request of a new one (see the removeIfClosed method).
> 
> The getSession methods are deprecated since they return the actual ZK connection objects
instead of sessions. In their place are open methods. The deprecated methods do not participate
in reference counting.
> 
> ZooSession now includes a clear method for simply wiping out its session tracking and
a forceCloseAll method for forcibly closing all known connections. These methods are useful
for ensuring that connections are wiped clean, so that it isn't necessary to hunt down and
close all sessions before, say, a process exits.
> 
> ZooReader was modified to remember the session that it is using (with its reference to
the ZK connection) instead of continuously asking ZooSession for it. When a ZooReader is closed,
it closes its session instead of closing the ZK connection itself; this allows the reference
counting to work.
> 
> This set of commits adds a unit test for ZooSession. ZooSession is defined to implement
Closeable, but ZooReader is not.
> 
> 
> Diffs
> -----
> 
>   src/core/pom.xml 77a4a72b3d32283e82d7a9d3b1b6b4690520c88e 
>   src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java cbb39182e93c568363a58d7e3344b9b85337cfc0

>   src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooKeeperFactory.java PRE-CREATION

>   src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooKeeperFactoryException.java
PRE-CREATION 
>   src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooReader.java aabc0f223bf2c6187e915b9a1e9569d714bce852

>   src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooSession.java b3db26f107139c1e2854a545e3f1f11de1b28b02

>   src/core/src/test/java/org/apache/accumulo/core/zookeeper/ZooSessionTest.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/16485/diff/
> 
> 
> Testing
> -------
> 
> Unit tests; functional tests; continuous ingest.
> 
> 
> Thanks,
> 
> Bill Havanki
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message