Re: Review Request 48757: GEODE-11: Cleaning up old IndexRepositoryImpls when buckets move

2016-06-24 Thread Dan Smith 

---
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)


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

2016-06-23 Thread anilkumar gingade 

---
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)


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

2016-06-21 Thread Dan Smith 

---
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

2016-06-15 Thread anilkumar gingade 

---
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)


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

2016-06-15 Thread Jason Huynh 

---
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)


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

2016-06-15 Thread Dan Smith 

---
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

Re: Review Request 48757: GEODE-11: Cleaning up old IndexRepositoryImpls when buckets move

2016-06-15 Thread Dan Smith 


> 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
> > 
> >
> > 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
> 
>

Review Request 48757: GEODE-11: Cleaning up old IndexRepositoryImpls when buckets move

2016-06-15 Thread Dan Smith 

---
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