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/03 01:43:52 UTC
Review Request 46918: GEODE-11: Refactoring the
LuceneFunctionReadPathDUnitTest
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46918/
-----------------------------------------------------------
Review request for geode, Jason Huynh, nabarun nag, and xiaojian zhou.
Repository: geode
Description
-------
Refactoring this test into a framework for adding more tests with a
bunch of subclasses.
Diffs
-----
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPR.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/LuceneFunctionReadPathDUnitTest.java f6fcf8a36ea73218918c93192a98c60eae83ac0b
Diff: https://reviews.apache.org/r/46918/diff/
Testing
-------
Thanks,
Dan Smith
Re: Review Request 46918: GEODE-11: Refactoring the
LuceneFunctionReadPathDUnitTest
Posted by Jason Huynh <hu...@gmail.com>.
> On May 3, 2016, 9:20 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java, line 60
> > <https://reviews.apache.org/r/46918/diff/2/?file=1369368#file1369368line60>
> >
> > Should we pull the "createIndex" into it's own method call and not have it be part of initDataStore?
> >
> > The only reason why we might want to do that would be when we finally support creating indexes after the region has been created?
>
> Dan Smith wrote:
> The reason why I made the createIndex code a callback is that I wanted the test class to be able to decide when and if it should run it. I guess maybe that made more sense for the accessor VM, because an accessor could be a datastore or it could be a client depending on the topology we're testing. So maybe I should take it out of the initDataStore but leave it in initAccessor? I think maybe I'll leave them the same for the moment but we can refactor if we need to add different tests for different orders?
Oh I just saw that the initDataStore for some of the classes would call createIndex.run() before creating the region but perhaps later on we would want tests in the same classes to created the index after the region was created. As you said, we can always refactor it later.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46918/#review131560
-----------------------------------------------------------
On May 3, 2016, 12:12 a.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46918/
> -----------------------------------------------------------
>
> (Updated May 3, 2016, 12:12 a.m.)
>
>
> Review request for geode, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Refactoring this test into a framework for adding more tests with a
> bunch of subclasses.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPRBase.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPR.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/LuceneFunctionReadPathDUnitTest.java f6fcf8a36ea73218918c93192a98c60eae83ac0b
>
> Diff: https://reviews.apache.org/r/46918/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 46918: GEODE-11: Refactoring the
LuceneFunctionReadPathDUnitTest
Posted by Dan Smith <ds...@pivotal.io>.
> On May 3, 2016, 9:20 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java, line 60
> > <https://reviews.apache.org/r/46918/diff/2/?file=1369368#file1369368line60>
> >
> > Should we pull the "createIndex" into it's own method call and not have it be part of initDataStore?
> >
> > The only reason why we might want to do that would be when we finally support creating indexes after the region has been created?
The reason why I made the createIndex code a callback is that I wanted the test class to be able to decide when and if it should run it. I guess maybe that made more sense for the accessor VM, because an accessor could be a datastore or it could be a client depending on the topology we're testing. So maybe I should take it out of the initDataStore but leave it in initAccessor? I think maybe I'll leave them the same for the moment but we can refactor if we need to add different tests for different orders?
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46918/#review131560
-----------------------------------------------------------
On May 3, 2016, 12:12 a.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46918/
> -----------------------------------------------------------
>
> (Updated May 3, 2016, 12:12 a.m.)
>
>
> Review request for geode, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Refactoring this test into a framework for adding more tests with a
> bunch of subclasses.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPRBase.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPR.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/LuceneFunctionReadPathDUnitTest.java f6fcf8a36ea73218918c93192a98c60eae83ac0b
>
> Diff: https://reviews.apache.org/r/46918/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 46918: GEODE-11: Refactoring the
LuceneFunctionReadPathDUnitTest
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46918/#review131560
-----------------------------------------------------------
Fix it, then Ship it!
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java (line 60)
<https://reviews.apache.org/r/46918/#comment195582>
Should we pull the "createIndex" into it's own method call and not have it be part of initDataStore?
The only reason why we might want to do that would be when we finally support creating indexes after the region has been created?
- Jason Huynh
On May 3, 2016, 12:12 a.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46918/
> -----------------------------------------------------------
>
> (Updated May 3, 2016, 12:12 a.m.)
>
>
> Review request for geode, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Refactoring this test into a framework for adding more tests with a
> bunch of subclasses.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPRBase.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPR.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/LuceneFunctionReadPathDUnitTest.java f6fcf8a36ea73218918c93192a98c60eae83ac0b
>
> Diff: https://reviews.apache.org/r/46918/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 46918: GEODE-11: Refactoring the
LuceneFunctionReadPathDUnitTest
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46918/
-----------------------------------------------------------
(Updated May 3, 2016, 12:12 a.m.)
Review request for geode, Jason Huynh, nabarun nag, and xiaojian zhou.
Changes
-------
Applying review comments from Jason. Putting the PR specific rebalance test into a separate base class. So tests common to all region types will go in LuceneQueriesBase, tests common to PRs will go in LuceneQueriesPRBase.
Repository: geode
Description
-------
Refactoring this test into a framework for adding more tests with a
bunch of subclasses.
Diffs (updated)
-----
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPRBase.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPR.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/LuceneFunctionReadPathDUnitTest.java f6fcf8a36ea73218918c93192a98c60eae83ac0b
Diff: https://reviews.apache.org/r/46918/diff/
Testing
-------
Thanks,
Dan Smith
Re: Review Request 46918: GEODE-11: Refactoring the
LuceneFunctionReadPathDUnitTest
Posted by Dan Smith <ds...@pivotal.io>.
> On May 2, 2016, 11:46 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java, line 26
> > <https://reviews.apache.org/r/46918/diff/1/?file=1369345#file1369345line26>
> >
> > Remove?
Will do.
> On May 2, 2016, 11:46 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java, line 101
> > <https://reviews.apache.org/r/46918/diff/1/?file=1369343#file1369343line101>
> >
> > For future reference, what happens if the region is a replicated region, does rebalance just get ignored?
Hmm, yeah, maybe we need a better heirarchy of tests. PR specific tests should be in a subclass.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46918/#review131422
-----------------------------------------------------------
On May 2, 2016, 11:43 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46918/
> -----------------------------------------------------------
>
> (Updated May 2, 2016, 11:43 p.m.)
>
>
> Review request for geode, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Refactoring this test into a framework for adding more tests with a
> bunch of subclasses.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPR.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/LuceneFunctionReadPathDUnitTest.java f6fcf8a36ea73218918c93192a98c60eae83ac0b
>
> Diff: https://reviews.apache.org/r/46918/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>
Re: Review Request 46918: GEODE-11: Refactoring the
LuceneFunctionReadPathDUnitTest
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46918/#review131422
-----------------------------------------------------------
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java (line 101)
<https://reviews.apache.org/r/46918/#comment195396>
For future reference, what happens if the region is a replicated region, does rebalance just get ignored?
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java (line 26)
<https://reviews.apache.org/r/46918/#comment195397>
Remove?
- Jason Huynh
On May 2, 2016, 11:43 p.m., Dan Smith wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46918/
> -----------------------------------------------------------
>
> (Updated May 2, 2016, 11:43 p.m.)
>
>
> Review request for geode, Jason Huynh, nabarun nag, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Refactoring this test into a framework for adding more tests with a
> bunch of subclasses.
>
>
> Diffs
> -----
>
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesBase.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPR.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/LuceneQueriesPeerPROverflow.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/distributed/LuceneFunctionReadPathDUnitTest.java f6fcf8a36ea73218918c93192a98c60eae83ac0b
>
> Diff: https://reviews.apache.org/r/46918/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dan Smith
>
>