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/05/26 18:26:07 UTC
Review Request 47912: GEODE-11: Adding stats for lucene index updates
and queries
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47912/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, nabarun nag, and xiaojian zhou.
Repository: geode
Description
-------
Adding some initial stats for lucene indexes tracking updates and queries.
I haven't yet figured out how to update the documents stat, but I figured I would get the easy stats checked in first. We're tracking updates, commits, and query stats with this change.
Diffs
-----
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexForPartitionedRegion.java d22ca4a196df3b1a457b56c92da694bdbf792cc2
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java c165085c83b38930bb970ac8f8e3f3fd8aa8808f
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java PRE-CREATION
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 57b8862d91cb0b825de27d67ff594c984fc8ca55
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 065cc6a5c2b140a2fb383362d9e8d1316e903e8a
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexRecoveryHAIntegrationTest.java 194f3c7adfa32d0f2c8e9f18a27a3217d488560c
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java 4532d1651845c429bfa2b9f2f4eebf9e6e2bbdb2
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/DistributedScoringJUnitTest.java 7bcc7619198c360d0ec3a5d887bb0b2b4a483d8d
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 53c41616042dfe71febe7bc192c74f26f3938203
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplPerformanceTest.java cec76ba0533093c88d653879323acba4e1901c9e
Diff: https://reviews.apache.org/r/47912/diff/
Testing
-------
Thanks,
Dan Smith
Re: Review Request 47912: GEODE-11: Adding stats for lucene index
updates and queries
Posted by Dan Smith <ds...@pivotal.io>.
> On May 26, 2016, 6:53 p.m., xiaojian zhou wrote:
> > geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java, line 92
> > <https://reviews.apache.org/r/47912/diff/1/?file=1395637#file1395637line92>
> >
> > I'm concerning about "stats" showed up in many constructor methods.
> >
> > The stats object should be keeped in LuceneIndexImpl object, then we use index object to reference it in other classes, such as RepositoryManager.
Hmm, this might be a matter of personal taste. I like having the dependencies explicitly listed in the contructor, because it makes it easier to write tests. If we pass one big index object everywhere, tests of PartitionRepositoryManager and IndexRepository have to mock the index and then mock the stats as well.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47912/#review135044
-----------------------------------------------------------
On May 26, 2016, 6:26 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47912/
> -----------------------------------------------------------
>
> (Updated May 26, 2016, 6:26 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Adding some initial stats for lucene indexes tracking updates and queries.
>
> I haven't yet figured out how to update the documents stat, but I figured I would get the easy stats checked in first. We're tracking updates, commits, and query stats with this change.
>
>
> Diffs
> -----
>
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexForPartitionedRegion.java d22ca4a196df3b1a457b56c92da694bdbf792cc2
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java c165085c83b38930bb970ac8f8e3f3fd8aa8808f
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java PRE-CREATION
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 57b8862d91cb0b825de27d67ff594c984fc8ca55
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 065cc6a5c2b140a2fb383362d9e8d1316e903e8a
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexRecoveryHAIntegrationTest.java 194f3c7adfa32d0f2c8e9f18a27a3217d488560c
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java 4532d1651845c429bfa2b9f2f4eebf9e6e2bbdb2
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/DistributedScoringJUnitTest.java 7bcc7619198c360d0ec3a5d887bb0b2b4a483d8d
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 53c41616042dfe71febe7bc192c74f26f3938203
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplPerformanceTest.java cec76ba0533093c88d653879323acba4e1901c9e
>
> Diff: https://reviews.apache.org/r/47912/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 47912: GEODE-11: Adding stats for lucene index
updates and queries
Posted by xiaojian zhou <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47912/#review135044
-----------------------------------------------------------
geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java (line 92)
<https://reviews.apache.org/r/47912/#comment200025>
I'm concerning about "stats" showed up in many constructor methods.
The stats object should be keeped in LuceneIndexImpl object, then we use index object to reference it in other classes, such as RepositoryManager.
- xiaojian zhou
On May 26, 2016, 6:26 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47912/
> -----------------------------------------------------------
>
> (Updated May 26, 2016, 6:26 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Adding some initial stats for lucene indexes tracking updates and queries.
>
> I haven't yet figured out how to update the documents stat, but I figured I would get the easy stats checked in first. We're tracking updates, commits, and query stats with this change.
>
>
> Diffs
> -----
>
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexForPartitionedRegion.java d22ca4a196df3b1a457b56c92da694bdbf792cc2
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java c165085c83b38930bb970ac8f8e3f3fd8aa8808f
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java PRE-CREATION
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 57b8862d91cb0b825de27d67ff594c984fc8ca55
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 065cc6a5c2b140a2fb383362d9e8d1316e903e8a
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexRecoveryHAIntegrationTest.java 194f3c7adfa32d0f2c8e9f18a27a3217d488560c
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java 4532d1651845c429bfa2b9f2f4eebf9e6e2bbdb2
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/DistributedScoringJUnitTest.java 7bcc7619198c360d0ec3a5d887bb0b2b4a483d8d
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 53c41616042dfe71febe7bc192c74f26f3938203
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplPerformanceTest.java cec76ba0533093c88d653879323acba4e1901c9e
>
> Diff: https://reviews.apache.org/r/47912/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 47912: GEODE-11: Adding stats for lucene index
updates and queries
Posted by Dan Smith <ds...@pivotal.io>.
> On May 26, 2016, 8:10 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java, line 116
> > <https://reviews.apache.org/r/47912/diff/1/?file=1395643#file1395643line116>
> >
> > Not really an issue, but rather a test maintenance question; are we planning on adding to this test when a new stat is added or should we break this up into maybe the crud stats, and query? If we break it up per stat, I can see why that would be a bit tedious...
I'll split it into a test per stat - I think you are right, that is cleaner.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47912/#review135056
-----------------------------------------------------------
On May 26, 2016, 6:26 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47912/
> -----------------------------------------------------------
>
> (Updated May 26, 2016, 6:26 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Adding some initial stats for lucene indexes tracking updates and queries.
>
> I haven't yet figured out how to update the documents stat, but I figured I would get the easy stats checked in first. We're tracking updates, commits, and query stats with this change.
>
>
> Diffs
> -----
>
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexForPartitionedRegion.java d22ca4a196df3b1a457b56c92da694bdbf792cc2
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java c165085c83b38930bb970ac8f8e3f3fd8aa8808f
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java PRE-CREATION
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 57b8862d91cb0b825de27d67ff594c984fc8ca55
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 065cc6a5c2b140a2fb383362d9e8d1316e903e8a
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexRecoveryHAIntegrationTest.java 194f3c7adfa32d0f2c8e9f18a27a3217d488560c
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java 4532d1651845c429bfa2b9f2f4eebf9e6e2bbdb2
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/DistributedScoringJUnitTest.java 7bcc7619198c360d0ec3a5d887bb0b2b4a483d8d
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 53c41616042dfe71febe7bc192c74f26f3938203
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplPerformanceTest.java cec76ba0533093c88d653879323acba4e1901c9e
>
> Diff: https://reviews.apache.org/r/47912/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 47912: GEODE-11: Adding stats for lucene index
updates and queries
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47912/#review135056
-----------------------------------------------------------
Fix it, then Ship it!
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java (line 116)
<https://reviews.apache.org/r/47912/#comment200031>
Not really an issue, but rather a test maintenance question; are we planning on adding to this test when a new stat is added or should we break this up into maybe the crud stats, and query? If we break it up per stat, I can see why that would be a bit tedious...
- Jason Huynh
On May 26, 2016, 6:26 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47912/
> -----------------------------------------------------------
>
> (Updated May 26, 2016, 6:26 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Adding some initial stats for lucene indexes tracking updates and queries.
>
> I haven't yet figured out how to update the documents stat, but I figured I would get the easy stats checked in first. We're tracking updates, commits, and query stats with this change.
>
>
> Diffs
> -----
>
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexForPartitionedRegion.java d22ca4a196df3b1a457b56c92da694bdbf792cc2
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexImpl.java c165085c83b38930bb970ac8f8e3f3fd8aa8808f
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStats.java PRE-CREATION
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManager.java 57b8862d91cb0b825de27d67ff594c984fc8ca55
> geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImpl.java 065cc6a5c2b140a2fb383362d9e8d1316e903e8a
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexRecoveryHAIntegrationTest.java 194f3c7adfa32d0f2c8e9f18a27a3217d488560c
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexStatsJUnitTest.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/PartitionedRepositoryManagerJUnitTest.java 4532d1651845c429bfa2b9f2f4eebf9e6e2bbdb2
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/DistributedScoringJUnitTest.java 7bcc7619198c360d0ec3a5d887bb0b2b4a483d8d
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplJUnitTest.java 53c41616042dfe71febe7bc192c74f26f3938203
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/repository/IndexRepositoryImplPerformanceTest.java cec76ba0533093c88d653879323acba4e1901c9e
>
> Diff: https://reviews.apache.org/r/47912/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>