You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Dan Smith <ds...@pivotal.io> on 2016/06/15 21:13:43 UTC
Review Request 48757: GEODE-11: Cleaning up old IndexRepositoryImpls
when buckets move
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48757/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Jason Huynh, nabarun nag, and xiaojian zhou.
Repository: geode
Description
-------
When a bucket is moved, we leave the IndexRepositoryImpl open. But even
after the bucket moves back, we just dereference the old
IndexRepositoryImpl without closing it. We should make sure we always
invoke close on the IndexRepositoryImpl to clean up any resources the
IndexWriter is using.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 95854ec2b47e82be946315ee65218fe504075b79
geode-core/src/test/java/com/gemstone/gemfire/test/fake/Fakes.java 323c281baaaf10bcf17c4b421b333de52f08dccd
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 25010b04cf10d6216b91a8de29b8c92ea2db34bf
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepository.java fab2c2a5df17f836c29d983a41632469354d3955
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 110f85acd27a7c958357074ee0d30dccdf763567
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java ec56381bb54baa2de3921850afbb659c7ecf8fc8
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 7d3caf89b42a72dc0f8a9d934578e28cb4a95731
Diff: https://reviews.apache.org/r/48757/diff/
Testing
-------
Thanks,
Dan Smith
Re: Review Request 48757: GEODE-11: Cleaning up old
IndexRepositoryImpls when buckets move
Posted by Dan Smith <ds...@pivotal.io>.
> On June 15, 2016, 9:28 p.m., Jason Huynh wrote:
> > geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java, line 62
> > <https://reviews.apache.org/r/48757/diff/1/?file=1420398#file1420398line62>
> >
> > I think the method spelling is incorrect. Has one too many p's.... Not from this checkin but maybe we can fix that?
Sure thing, I'll fix that.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48757/#review137854
-----------------------------------------------------------
On June 15, 2016, 9:13 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48757/
> -----------------------------------------------------------
>
> (Updated June 15, 2016, 9:13 p.m.)
>
>
> Review request for geode, anilkumar gingade, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When a bucket is moved, we leave the IndexRepositoryImpl open. But even
> after the bucket moves back, we just dereference the old
> IndexRepositoryImpl without closing it. We should make sure we always
> invoke close on the IndexRepositoryImpl to clean up any resources the
> IndexWriter is using.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 95854ec2b47e82be946315ee65218fe504075b79
> geode-core/src/test/java/com/gemstone/gemfire/test/fake/Fakes.java 323c281baaaf10bcf17c4b421b333de52f08dccd
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 25010b04cf10d6216b91a8de29b8c92ea2db34bf
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepository.java fab2c2a5df17f836c29d983a41632469354d3955
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 110f85acd27a7c958357074ee0d30dccdf763567
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java ec56381bb54baa2de3921850afbb659c7ecf8fc8
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 7d3caf89b42a72dc0f8a9d934578e28cb4a95731
>
> Diff: https://reviews.apache.org/r/48757/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 48757: GEODE-11: Cleaning up old
IndexRepositoryImpls when buckets move
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48757/#review137854
-----------------------------------------------------------
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java (line 62)
<https://reviews.apache.org/r/48757/#comment203048>
I think the method spelling is incorrect. Has one too many p's.... Not from this checkin but maybe we can fix that?
- Jason Huynh
On June 15, 2016, 9:13 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48757/
> -----------------------------------------------------------
>
> (Updated June 15, 2016, 9:13 p.m.)
>
>
> Review request for geode, anilkumar gingade, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When a bucket is moved, we leave the IndexRepositoryImpl open. But even
> after the bucket moves back, we just dereference the old
> IndexRepositoryImpl without closing it. We should make sure we always
> invoke close on the IndexRepositoryImpl to clean up any resources the
> IndexWriter is using.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 95854ec2b47e82be946315ee65218fe504075b79
> geode-core/src/test/java/com/gemstone/gemfire/test/fake/Fakes.java 323c281baaaf10bcf17c4b421b333de52f08dccd
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 25010b04cf10d6216b91a8de29b8c92ea2db34bf
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepository.java fab2c2a5df17f836c29d983a41632469354d3955
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 110f85acd27a7c958357074ee0d30dccdf763567
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java ec56381bb54baa2de3921850afbb659c7ecf8fc8
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 7d3caf89b42a72dc0f8a9d934578e28cb4a95731
>
> Diff: https://reviews.apache.org/r/48757/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 48757: GEODE-11: Cleaning up old
IndexRepositoryImpls when buckets move
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48757/#review139313
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java (line 153)
<https://reviews.apache.org/r/48757/#comment204492>
Don't we need to set the newly created repo to oldRepository, so that the IndexRepositories map gets updated with this.
The mutliple threads can still execute this concurrently...Will that be an issue?
- anilkumar gingade
On June 21, 2016, 5:40 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48757/
> -----------------------------------------------------------
>
> (Updated June 21, 2016, 5:40 p.m.)
>
>
> Review request for geode, anilkumar gingade, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When a bucket is moved, we leave the IndexRepositoryImpl open. But even
> after the bucket moves back, we just dereference the old
> IndexRepositoryImpl without closing it. We should make sure we always
> invoke close on the IndexRepositoryImpl to clean up any resources the
> IndexWriter is using.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 95854ec2b47e82be946315ee65218fe504075b79
> geode-core/src/test/java/com/gemstone/gemfire/test/fake/Fakes.java 323c281baaaf10bcf17c4b421b333de52f08dccd
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java ea1f35e57da557bcc298356f80d34165dc3d633a
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 25010b04cf10d6216b91a8de29b8c92ea2db34bf
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepository.java fab2c2a5df17f836c29d983a41632469354d3955
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 110f85acd27a7c958357074ee0d30dccdf763567
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java 05e64afd1719f5d56b71c964756b425530f6a399
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java ec56381bb54baa2de3921850afbb659c7ecf8fc8
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 7d3caf89b42a72dc0f8a9d934578e28cb4a95731
>
> Diff: https://reviews.apache.org/r/48757/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 48757: GEODE-11: Cleaning up old
IndexRepositoryImpls when buckets move
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48757/#review139408
-----------------------------------------------------------
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java (line 153)
<https://reviews.apache.org/r/48757/#comment204633>
RE multiple threads - the ConcurrentHashMap.compute gaurantees that the update will be executed atomically.
RE setting to oldRepository, I'm not quite sure what you mean? By returning the newly created repo from compute, we're telling ConcurrentHashMap to store that value in the map.
- Dan Smith
On June 21, 2016, 5:40 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48757/
> -----------------------------------------------------------
>
> (Updated June 21, 2016, 5:40 p.m.)
>
>
> Review request for geode, anilkumar gingade, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When a bucket is moved, we leave the IndexRepositoryImpl open. But even
> after the bucket moves back, we just dereference the old
> IndexRepositoryImpl without closing it. We should make sure we always
> invoke close on the IndexRepositoryImpl to clean up any resources the
> IndexWriter is using.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 95854ec2b47e82be946315ee65218fe504075b79
> geode-core/src/test/java/com/gemstone/gemfire/test/fake/Fakes.java 323c281baaaf10bcf17c4b421b333de52f08dccd
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java ea1f35e57da557bcc298356f80d34165dc3d633a
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 25010b04cf10d6216b91a8de29b8c92ea2db34bf
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepository.java fab2c2a5df17f836c29d983a41632469354d3955
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 110f85acd27a7c958357074ee0d30dccdf763567
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java 05e64afd1719f5d56b71c964756b425530f6a399
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java ec56381bb54baa2de3921850afbb659c7ecf8fc8
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 7d3caf89b42a72dc0f8a9d934578e28cb4a95731
>
> Diff: https://reviews.apache.org/r/48757/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 48757: GEODE-11: Cleaning up old
IndexRepositoryImpls when buckets move
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48757/
-----------------------------------------------------------
(Updated June 21, 2016, 5:40 p.m.)
Review request for geode, anilkumar gingade, Jason Huynh, nabarun nag, and xiaojian zhou.
Changes
-------
Changing the way IndexRepositories are created, based on review feedback. Instead of logic that may create a duplicate repository and then clean it up if putIfAbsent fails, the getRepository method now uses the ConcurrentHashMap.compute to create the IndexRepository atomically under a lock.
Repository: geode
Description
-------
When a bucket is moved, we leave the IndexRepositoryImpl open. But even
after the bucket moves back, we just dereference the old
IndexRepositoryImpl without closing it. We should make sure we always
invoke close on the IndexRepositoryImpl to clean up any resources the
IndexWriter is using.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 95854ec2b47e82be946315ee65218fe504075b79
geode-core/src/test/java/com/gemstone/gemfire/test/fake/Fakes.java 323c281baaaf10bcf17c4b421b333de52f08dccd
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java ea1f35e57da557bcc298356f80d34165dc3d633a
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 25010b04cf10d6216b91a8de29b8c92ea2db34bf
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepository.java fab2c2a5df17f836c29d983a41632469354d3955
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 110f85acd27a7c958357074ee0d30dccdf763567
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java 05e64afd1719f5d56b71c964756b425530f6a399
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java ec56381bb54baa2de3921850afbb659c7ecf8fc8
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 7d3caf89b42a72dc0f8a9d934578e28cb4a95731
Diff: https://reviews.apache.org/r/48757/diff/
Testing
-------
Thanks,
Dan Smith
Re: Review Request 48757: GEODE-11: Cleaning up old
IndexRepositoryImpls when buckets move
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48757/#review137879
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java (line 139)
<https://reviews.apache.org/r/48757/#comment203083>
Synchronization...To avoid multiple threads creating the repo.
- anilkumar gingade
On June 15, 2016, 9:36 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48757/
> -----------------------------------------------------------
>
> (Updated June 15, 2016, 9:36 p.m.)
>
>
> Review request for geode, anilkumar gingade, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When a bucket is moved, we leave the IndexRepositoryImpl open. But even
> after the bucket moves back, we just dereference the old
> IndexRepositoryImpl without closing it. We should make sure we always
> invoke close on the IndexRepositoryImpl to clean up any resources the
> IndexWriter is using.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 95854ec2b47e82be946315ee65218fe504075b79
> geode-core/src/test/java/com/gemstone/gemfire/test/fake/Fakes.java 323c281baaaf10bcf17c4b421b333de52f08dccd
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java ea1f35e57da557bcc298356f80d34165dc3d633a
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 25010b04cf10d6216b91a8de29b8c92ea2db34bf
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepository.java fab2c2a5df17f836c29d983a41632469354d3955
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 110f85acd27a7c958357074ee0d30dccdf763567
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java 05e64afd1719f5d56b71c964756b425530f6a399
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java ec56381bb54baa2de3921850afbb659c7ecf8fc8
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 7d3caf89b42a72dc0f8a9d934578e28cb4a95731
>
> Diff: https://reviews.apache.org/r/48757/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 48757: GEODE-11: Cleaning up old
IndexRepositoryImpls when buckets move
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48757/
-----------------------------------------------------------
(Updated June 15, 2016, 9:36 p.m.)
Review request for geode, anilkumar gingade, Jason Huynh, nabarun nag, and xiaojian zhou.
Changes
-------
Fixing the spelling of addDocumentsSupplier.
Repository: geode
Description
-------
When a bucket is moved, we leave the IndexRepositoryImpl open. But even
after the bucket moves back, we just dereference the old
IndexRepositoryImpl without closing it. We should make sure we always
invoke close on the IndexRepositoryImpl to clean up any resources the
IndexWriter is using.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegion.java 95854ec2b47e82be946315ee65218fe504075b79
geode-core/src/test/java/com/gemstone/gemfire/test/fake/Fakes.java 323c281baaaf10bcf17c4b421b333de52f08dccd
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java ea1f35e57da557bcc298356f80d34165dc3d633a
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 25010b04cf10d6216b91a8de29b8c92ea2db34bf
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepository.java fab2c2a5df17f836c29d983a41632469354d3955
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 110f85acd27a7c958357074ee0d30dccdf763567
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java 05e64afd1719f5d56b71c964756b425530f6a399
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java ec56381bb54baa2de3921850afbb659c7ecf8fc8
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 7d3caf89b42a72dc0f8a9d934578e28cb4a95731
Diff: https://reviews.apache.org/r/48757/diff/
Testing
-------
Thanks,
Dan Smith