You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Sean Busbey <se...@manvsbeard.com> on 2013/11/13 17:11:07 UTC
Review Request 15481: ACCUMULO-1858 Backport fix for ACCUMULO-1379 PermGen
Leak to 1.4 and 1.5
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15481/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-1858
https://issues.apache.org/jira/browse/ACCUMULO-1858
Repository: accumulo
Description
-------
ACCUMULO-1379 Fix edge cases if error in closing ZooKeeperInstance
(cherry picked from commit 3f6c66ede52cb1fb5a122d7bad06d7978ff0a671)
Reason: bugfix
Author: Christopher Tubbs <ct...@apache.org>
Ref: ACCUMULO-1858
ACCUMULO-1379 - Adding close() to Instance to assist in freeing up resources
(cherry picked from commit 7da1164d87227960d3e0cfc841f753067e2c0304)
Reason: bugfix
Author: John Vines <jv...@gmail.com>
Ref: ACCUMULO-1858
Diffs
-----
src/core/src/main/java/org/apache/accumulo/core/client/Instance.java b3d09ba30b1175a495c9d0cb55b0b4a4f3aadbe1
src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java f657c07a28c3bf330556f0d4e02b9adcb0ea755e
src/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java ef3724b149921b06cc27945eca11e4d0d1658c6b
src/core/src/main/java/org/apache/accumulo/core/client/mock/MockInstance.java 2ff7b8211de031a4fa04cae3c5bd7a3e03d50506
src/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java 1b1cdd735bbf9772dc1a7f82e337125e675aa7b2
src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java f5bdd6b71652e8e2d664ca2d6480921f11663214
src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooReader.java 47663acbc421b890bc42bac2d7d5c5556119e6a8
src/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java 538cb6c700423926cd1789654c2933a8ccbb1d65
src/server/src/main/java/org/apache/accumulo/server/client/HdfsZooInstance.java e6cdb6397abe08883dc92700cbe2dc7ce7af6e1a
Diff: https://reviews.apache.org/r/15481/diff/
Testing
-------
Unit tests pass. Functional tests in progress.
Thanks,
Sean Busbey
Re: Review Request 15481: ACCUMULO-1858 Backport fix for ACCUMULO-1379
PermGen Leak to 1.4 and 1.5
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Nov. 13, 2013, 4:19 p.m., Sean Busbey wrote:
> > src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, lines 300-311
> > <https://reviews.apache.org/r/15481/diff/1/?file=383542#file383542line300>
> >
> > I think moving the assignment of closed = true into the if block is incorrect.
> >
> > Doing so allows a given client to decrement the client count multiple times on subsequent calls to close(), which is incorrect.
> >
> > It also allows continued access to a ZKI that has been closed so long as there are additional ZKIs around.
> >
> > In the event that we enter the InterruptedException handling block, we'll avoid setting this ZKI to closed wether the assignment is in the if block or outside of it.
> >
> > This probably needs to be handled in a follow-on jira so that the fix can apply to this backport and then forward to 1.6.
Filed ACCUMULO-1889
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15481/#review28809
-----------------------------------------------------------
On Nov. 13, 2013, 4:11 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15481/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2013, 4:11 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1858
> https://issues.apache.org/jira/browse/ACCUMULO-1858
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> ACCUMULO-1379 Fix edge cases if error in closing ZooKeeperInstance
>
> (cherry picked from commit 3f6c66ede52cb1fb5a122d7bad06d7978ff0a671)
>
> Reason: bugfix
> Author: Christopher Tubbs <ct...@apache.org>
> Ref: ACCUMULO-1858
>
> ACCUMULO-1379 - Adding close() to Instance to assist in freeing up resources
>
> (cherry picked from commit 7da1164d87227960d3e0cfc841f753067e2c0304)
>
> Reason: bugfix
> Author: John Vines <jv...@gmail.com>
> Ref: ACCUMULO-1858
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/Instance.java b3d09ba30b1175a495c9d0cb55b0b4a4f3aadbe1
> src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java f657c07a28c3bf330556f0d4e02b9adcb0ea755e
> src/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java ef3724b149921b06cc27945eca11e4d0d1658c6b
> src/core/src/main/java/org/apache/accumulo/core/client/mock/MockInstance.java 2ff7b8211de031a4fa04cae3c5bd7a3e03d50506
> src/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java 1b1cdd735bbf9772dc1a7f82e337125e675aa7b2
> src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java f5bdd6b71652e8e2d664ca2d6480921f11663214
> src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooReader.java 47663acbc421b890bc42bac2d7d5c5556119e6a8
> src/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java 538cb6c700423926cd1789654c2933a8ccbb1d65
> src/server/src/main/java/org/apache/accumulo/server/client/HdfsZooInstance.java e6cdb6397abe08883dc92700cbe2dc7ce7af6e1a
>
> Diff: https://reviews.apache.org/r/15481/diff/
>
>
> Testing
> -------
>
> Unit tests pass. Functional tests in progress.
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 15481: ACCUMULO-1858 Backport fix for ACCUMULO-1379
PermGen Leak to 1.4 and 1.5
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15481/#review28809
-----------------------------------------------------------
src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
<https://reviews.apache.org/r/15481/#comment55812>
I think moving the assignment of closed = true into the if block is incorrect.
Doing so allows a given client to decrement the client count multiple times on subsequent calls to close(), which is incorrect.
It also allows continued access to a ZKI that has been closed so long as there are additional ZKIs around.
In the event that we enter the InterruptedException handling block, we'll avoid setting this ZKI to closed wether the assignment is in the if block or outside of it.
This probably needs to be handled in a follow-on jira so that the fix can apply to this backport and then forward to 1.6.
- Sean Busbey
On Nov. 13, 2013, 4:11 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15481/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2013, 4:11 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1858
> https://issues.apache.org/jira/browse/ACCUMULO-1858
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> ACCUMULO-1379 Fix edge cases if error in closing ZooKeeperInstance
>
> (cherry picked from commit 3f6c66ede52cb1fb5a122d7bad06d7978ff0a671)
>
> Reason: bugfix
> Author: Christopher Tubbs <ct...@apache.org>
> Ref: ACCUMULO-1858
>
> ACCUMULO-1379 - Adding close() to Instance to assist in freeing up resources
>
> (cherry picked from commit 7da1164d87227960d3e0cfc841f753067e2c0304)
>
> Reason: bugfix
> Author: John Vines <jv...@gmail.com>
> Ref: ACCUMULO-1858
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/Instance.java b3d09ba30b1175a495c9d0cb55b0b4a4f3aadbe1
> src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java f657c07a28c3bf330556f0d4e02b9adcb0ea755e
> src/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java ef3724b149921b06cc27945eca11e4d0d1658c6b
> src/core/src/main/java/org/apache/accumulo/core/client/mock/MockInstance.java 2ff7b8211de031a4fa04cae3c5bd7a3e03d50506
> src/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java 1b1cdd735bbf9772dc1a7f82e337125e675aa7b2
> src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java f5bdd6b71652e8e2d664ca2d6480921f11663214
> src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooReader.java 47663acbc421b890bc42bac2d7d5c5556119e6a8
> src/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java 538cb6c700423926cd1789654c2933a8ccbb1d65
> src/server/src/main/java/org/apache/accumulo/server/client/HdfsZooInstance.java e6cdb6397abe08883dc92700cbe2dc7ce7af6e1a
>
> Diff: https://reviews.apache.org/r/15481/diff/
>
>
> Testing
> -------
>
> Unit tests pass. Functional tests in progress.
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 15481: ACCUMULO-1858 Backport fix for ACCUMULO-1379
PermGen Leak to 1.4 and 1.5
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Nov. 13, 2013, 6:18 p.m., Bill Havanki wrote:
> > (non-binding) Appears to be a correct backport. The changes to ConditionalWriterTest do not apply, as the test does not appear before 1.6.x.
I should probably note that in the commit message. good catch.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15481/#review28817
-----------------------------------------------------------
On Nov. 13, 2013, 4:11 p.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15481/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2013, 4:11 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1858
> https://issues.apache.org/jira/browse/ACCUMULO-1858
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> ACCUMULO-1379 Fix edge cases if error in closing ZooKeeperInstance
>
> (cherry picked from commit 3f6c66ede52cb1fb5a122d7bad06d7978ff0a671)
>
> Reason: bugfix
> Author: Christopher Tubbs <ct...@apache.org>
> Ref: ACCUMULO-1858
>
> ACCUMULO-1379 - Adding close() to Instance to assist in freeing up resources
>
> (cherry picked from commit 7da1164d87227960d3e0cfc841f753067e2c0304)
>
> Reason: bugfix
> Author: John Vines <jv...@gmail.com>
> Ref: ACCUMULO-1858
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/Instance.java b3d09ba30b1175a495c9d0cb55b0b4a4f3aadbe1
> src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java f657c07a28c3bf330556f0d4e02b9adcb0ea755e
> src/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java ef3724b149921b06cc27945eca11e4d0d1658c6b
> src/core/src/main/java/org/apache/accumulo/core/client/mock/MockInstance.java 2ff7b8211de031a4fa04cae3c5bd7a3e03d50506
> src/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java 1b1cdd735bbf9772dc1a7f82e337125e675aa7b2
> src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java f5bdd6b71652e8e2d664ca2d6480921f11663214
> src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooReader.java 47663acbc421b890bc42bac2d7d5c5556119e6a8
> src/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java 538cb6c700423926cd1789654c2933a8ccbb1d65
> src/server/src/main/java/org/apache/accumulo/server/client/HdfsZooInstance.java e6cdb6397abe08883dc92700cbe2dc7ce7af6e1a
>
> Diff: https://reviews.apache.org/r/15481/diff/
>
>
> Testing
> -------
>
> Unit tests pass. Functional tests in progress.
>
>
> Thanks,
>
> Sean Busbey
>
>
Re: Review Request 15481: ACCUMULO-1858 Backport fix for ACCUMULO-1379
PermGen Leak to 1.4 and 1.5
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15481/#review28817
-----------------------------------------------------------
Ship it!
(non-binding) Appears to be a correct backport. The changes to ConditionalWriterTest do not apply, as the test does not appear before 1.6.x.
- Bill Havanki
On Nov. 13, 2013, 11:11 a.m., Sean Busbey wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15481/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2013, 11:11 a.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1858
> https://issues.apache.org/jira/browse/ACCUMULO-1858
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> ACCUMULO-1379 Fix edge cases if error in closing ZooKeeperInstance
>
> (cherry picked from commit 3f6c66ede52cb1fb5a122d7bad06d7978ff0a671)
>
> Reason: bugfix
> Author: Christopher Tubbs <ct...@apache.org>
> Ref: ACCUMULO-1858
>
> ACCUMULO-1379 - Adding close() to Instance to assist in freeing up resources
>
> (cherry picked from commit 7da1164d87227960d3e0cfc841f753067e2c0304)
>
> Reason: bugfix
> Author: John Vines <jv...@gmail.com>
> Ref: ACCUMULO-1858
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/Instance.java b3d09ba30b1175a495c9d0cb55b0b4a4f3aadbe1
> src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java f657c07a28c3bf330556f0d4e02b9adcb0ea755e
> src/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java ef3724b149921b06cc27945eca11e4d0d1658c6b
> src/core/src/main/java/org/apache/accumulo/core/client/mock/MockInstance.java 2ff7b8211de031a4fa04cae3c5bd7a3e03d50506
> src/core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java 1b1cdd735bbf9772dc1a7f82e337125e675aa7b2
> src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooCache.java f5bdd6b71652e8e2d664ca2d6480921f11663214
> src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooReader.java 47663acbc421b890bc42bac2d7d5c5556119e6a8
> src/core/src/test/java/org/apache/accumulo/core/client/impl/TabletLocatorImplTest.java 538cb6c700423926cd1789654c2933a8ccbb1d65
> src/server/src/main/java/org/apache/accumulo/server/client/HdfsZooInstance.java e6cdb6397abe08883dc92700cbe2dc7ce7af6e1a
>
> Diff: https://reviews.apache.org/r/15481/diff/
>
>
> Testing
> -------
>
> Unit tests pass. Functional tests in progress.
>
>
> Thanks,
>
> Sean Busbey
>
>