You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jason Huynh <hu...@gmail.com> on 2016/05/17 22:54:56 UTC
Review Request 47496: Adding transaction tests for lucene indexes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47496/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
Repository: geode
Description
-------
Added integration tests to test three scenarios:
1.) the lucene indexes are not updated during an in flight transaction (begin but before rollback or commit)
2.) the lucene indexes are updated after a commit
3.) the lucene indexes are not changed after a rollback
todo:
Do we want to add a dunit test for multiple vms and threads doing transactions? I avoided adding this test for now as I thought that was more of a "testing the geode transaction feature" rather than testing how the feature affects lucene.
Diffs
-----
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java fa3392c
Diff: https://reviews.apache.org/r/47496/diff/
Testing
-------
Thanks,
Jason Huynh
Re: Review Request 47496: Adding transaction tests for lucene indexes
Posted by Jason Huynh <hu...@gmail.com>.
> On May 18, 2016, 1 a.m., Dan Smith wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java, line 352
> > <https://reviews.apache.org/r/47496/diff/1/?file=1386143#file1386143line352>
> >
> > I'm actually surprised this works, since the field names are "title" and "description", not "text".
Looked into this a bit, apparently I need to use description:"\hello world\" as the string, otherwise lucene will look for the word "world" in the default field which was the reason why it was working the way it was. In our case the default I think are all fields or whatever the QueryParser is passed
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47496/#review133671
-----------------------------------------------------------
On May 17, 2016, 10:54 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47496/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 10:54 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added integration tests to test three scenarios:
> 1.) the lucene indexes are not updated during an in flight transaction (begin but before rollback or commit)
> 2.) the lucene indexes are updated after a commit
> 3.) the lucene indexes are not changed after a rollback
>
> todo:
> Do we want to add a dunit test for multiple vms and threads doing transactions? I avoided adding this test for now as I thought that was more of a "testing the geode transaction feature" rather than testing how the feature affects lucene.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java fa3392c
>
> Diff: https://reviews.apache.org/r/47496/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 47496: Adding transaction tests for lucene indexes
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47496/#review133671
-----------------------------------------------------------
Fix it, then Ship it!
Looks good!
Let's talk tomorrow a little be more about how we're organizing all of these integration tests. I had forgotten about this LuceneServiceImplIntegrationTest. I think the plan was to be adding integration tests like this:
LuceneIndexCreationIntegrationTest
LuceneIndexMaintainceIntegrationTest
LuceneQueryingIntegrationTest
Anyway, ship this and we can decide if we want to refactor the tests more later.
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java (line 337)
<https://reviews.apache.org/r/47496/#comment198244>
I'm actually surprised this works, since the field names are "title" and "description", not "text".
- Dan Smith
On May 17, 2016, 10:54 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47496/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 10:54 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added integration tests to test three scenarios:
> 1.) the lucene indexes are not updated during an in flight transaction (begin but before rollback or commit)
> 2.) the lucene indexes are updated after a commit
> 3.) the lucene indexes are not changed after a rollback
>
> todo:
> Do we want to add a dunit test for multiple vms and threads doing transactions? I avoided adding this test for now as I thought that was more of a "testing the geode transaction feature" rather than testing how the feature affects lucene.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java fa3392c
>
> Diff: https://reviews.apache.org/r/47496/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 47496: Adding transaction tests for lucene indexes
Posted by Jason Huynh <hu...@gmail.com>.
> On May 18, 2016, 12:16 a.m., anilkumar gingade wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java, line 321
> > <https://reviews.apache.org/r/47496/diff/1/?file=1386143#file1386143line321>
> >
> > How about moving these puts into region initialization method?
I left it this way for now because I think there will be cases where we want to create a region with different data and not necessarily have the same data every time. Each test can specifically tune the data they want to use and expect.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47496/#review133663
-----------------------------------------------------------
On May 17, 2016, 10:54 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47496/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 10:54 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added integration tests to test three scenarios:
> 1.) the lucene indexes are not updated during an in flight transaction (begin but before rollback or commit)
> 2.) the lucene indexes are updated after a commit
> 3.) the lucene indexes are not changed after a rollback
>
> todo:
> Do we want to add a dunit test for multiple vms and threads doing transactions? I avoided adding this test for now as I thought that was more of a "testing the geode transaction feature" rather than testing how the feature affects lucene.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java fa3392c
>
> Diff: https://reviews.apache.org/r/47496/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 47496: Adding transaction tests for lucene indexes
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47496/#review133663
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java (line 81)
<https://reviews.apache.org/r/47496/#comment198231>
LuceneTestUtilities has region and index name; in case if it can be used.
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java (line 112)
<https://reviews.apache.org/r/47496/#comment198232>
LuceneIntegrationTest has close cache...can we use/extend it?
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java (line 306)
<https://reviews.apache.org/r/47496/#comment198233>
How about moving these puts into region initialization method?
- anilkumar gingade
On May 17, 2016, 10:54 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47496/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 10:54 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added integration tests to test three scenarios:
> 1.) the lucene indexes are not updated during an in flight transaction (begin but before rollback or commit)
> 2.) the lucene indexes are updated after a commit
> 3.) the lucene indexes are not changed after a rollback
>
> todo:
> Do we want to add a dunit test for multiple vms and threads doing transactions? I avoided adding this test for now as I thought that was more of a "testing the geode transaction feature" rather than testing how the feature affects lucene.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java fa3392c
>
> Diff: https://reviews.apache.org/r/47496/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 47496: Adding transaction tests for lucene indexes
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47496/#review134033
-----------------------------------------------------------
Ship it!
Ship It!
- Dan Smith
On May 19, 2016, 6:32 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47496/
> -----------------------------------------------------------
>
> (Updated May 19, 2016, 6:32 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added integration tests to test three scenarios:
> 1.) the lucene indexes are not updated during an in flight transaction (begin but before rollback or commit)
> 2.) the lucene indexes are updated after a commit
> 3.) the lucene indexes are not changed after a rollback
>
> todo:
> Do we want to add a dunit test for multiple vms and threads doing transactions? I avoided adding this test for now as I thought that was more of a "testing the geode transaction feature" rather than testing how the feature affects lucene.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexCreationIntegrationTest.java 4c28938
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexCreationPersistenceIntegrationTest.java 23983cb
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexMaintenanceIntegrationTest.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIntegrationTest.java c302460
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java fa3392c
>
> Diff: https://reviews.apache.org/r/47496/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 47496: Adding transaction tests for lucene indexes
Posted by Barry Oglesby <bo...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47496/#review134034
-----------------------------------------------------------
Ship it!
Ship It!
- Barry Oglesby
On May 19, 2016, 6:32 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47496/
> -----------------------------------------------------------
>
> (Updated May 19, 2016, 6:32 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added integration tests to test three scenarios:
> 1.) the lucene indexes are not updated during an in flight transaction (begin but before rollback or commit)
> 2.) the lucene indexes are updated after a commit
> 3.) the lucene indexes are not changed after a rollback
>
> todo:
> Do we want to add a dunit test for multiple vms and threads doing transactions? I avoided adding this test for now as I thought that was more of a "testing the geode transaction feature" rather than testing how the feature affects lucene.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexCreationIntegrationTest.java 4c28938
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexCreationPersistenceIntegrationTest.java 23983cb
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexMaintenanceIntegrationTest.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIntegrationTest.java c302460
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java fa3392c
>
> Diff: https://reviews.apache.org/r/47496/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 47496: Adding transaction tests for lucene indexes
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47496/
-----------------------------------------------------------
(Updated May 19, 2016, 6:32 p.m.)
Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
Changes
-------
Updated based on review comments
Migrated a few tests from LuceneServiceImplIntegrationTest to better locations
Repository: geode
Description
-------
Added integration tests to test three scenarios:
1.) the lucene indexes are not updated during an in flight transaction (begin but before rollback or commit)
2.) the lucene indexes are updated after a commit
3.) the lucene indexes are not changed after a rollback
todo:
Do we want to add a dunit test for multiple vms and threads doing transactions? I avoided adding this test for now as I thought that was more of a "testing the geode transaction feature" rather than testing how the feature affects lucene.
Diffs (updated)
-----
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexCreationIntegrationTest.java 4c28938
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexCreationPersistenceIntegrationTest.java 23983cb
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIndexMaintenanceIntegrationTest.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneIntegrationTest.java c302460
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplIntegrationTest.java fa3392c
Diff: https://reviews.apache.org/r/47496/diff/
Testing
-------
Thanks,
Jason Huynh