You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by anilkumar gingade <ag...@pivotal.io> on 2016/08/11 22:34:09 UTC
Review Request 51010: GEODE-11 Test for Cluster Configuration changes
for Lucene index.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
Repository: geode
Description
-------
Added test to validate Cluster configuration support for Lucene indexes.
Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
Diffs (updated)
-----
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/51010/diff/
Testing
-------
Run the newly added test.
Thanks,
anilkumar gingade
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by anilkumar gingade <ag...@pivotal.io>.
> On Aug. 12, 2016, 4:28 p.m., Dan Smith wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java, line 49
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471613#file1471613line49>
> >
> > This could probably extend ExternalResource rather than directly implementing TestRule.
Changed to extend ExternalResource.
> On Aug. 12, 2016, 4:28 p.m., Dan Smith wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java, line 99
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471613#file1471613line99>
> >
> > I'm not sure getLocatorVM is the right thing to call this. It's not really getting the locator, it's actually configuring the locator. I suspect many tests you might not actually need to get the locator VM.
> >
> > Could the Rule just set this stuff up before the test runs, I wonder?
The idea here is to create cluster (locator, and nodes) using the given/custom properties...This simplifies setting up a cluster for those tests...We could change the Rule name appropriately, i could not come up with a better name....
There could be requirement for a test where it needs a vm configured with specified properties...We could add one more Rule that can do that...
> On Aug. 12, 2016, 4:28 p.m., Dan Smith wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java, line 113
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471613#file1471613line113>
> >
> > What if someone calls getVM instead of getNodeVM? I wonder if it would be possible to make this work so that someone doesn't need to call getNodeVM?
One could call getVm and configure it as they want (create a new DS)...And join the cluster using the locator port from the Locator configured in Rule.
> On Aug. 12, 2016, 4:28 p.m., Dan Smith wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java, line 122
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471613#file1471613line122>
> >
> > Use port 0 instead to avoid potential races. You can get the port back from the locator after it starts. See DUnitLauncher.startLocator.
Done.
> On Aug. 12, 2016, 4:28 p.m., Dan Smith wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java, line 129
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471613#file1471613line129>
> >
> > This could be a lambda: locator.invoke(() -> {...})
Done.
- anilkumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145629
-----------------------------------------------------------
On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 10:34 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145629
-----------------------------------------------------------
Looks good! I had a couple of comments about the LocatorServerConfigurationRule, see below.
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 49)
<https://reviews.apache.org/r/51010/#comment211933>
This could probably extend ExternalResource rather than directly implementing TestRule.
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 99)
<https://reviews.apache.org/r/51010/#comment211939>
I'm not sure getLocatorVM is the right thing to call this. It's not really getting the locator, it's actually configuring the locator. I suspect many tests you might not actually need to get the locator VM.
Could the Rule just set this stuff up before the test runs, I wonder?
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 113)
<https://reviews.apache.org/r/51010/#comment211935>
What if someone calls getVM instead of getNodeVM? I wonder if it would be possible to make this work so that someone doesn't need to call getNodeVM?
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 122)
<https://reviews.apache.org/r/51010/#comment211938>
Use port 0 instead to avoid potential races. You can get the port back from the locator after it starts. See DUnitLauncher.startLocator.
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 129)
<https://reviews.apache.org/r/51010/#comment211936>
This could be a lambda: locator.invoke(() -> {...})
- Dan Smith
On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 10:34 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145913
-----------------------------------------------------------
Ship it!
Looks good. I have some ideas on how we might be able to avoid having to call getLocatorVM, getNodeVM, etc. but I can always try to do that later. I'd say check it in!
- Dan Smith
On Aug. 12, 2016, 11:58 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2016, 11:58 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/
-----------------------------------------------------------
(Updated Aug. 12, 2016, 11:58 p.m.)
Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
Changes
-------
Made changes based on review comments. Extening ExternalResource. Using Lambda expression. Use of 0 for locator port. Seperating test for group and non-group members.
Repository: geode
Description
-------
Added test to validate Cluster configuration support for Lucene indexes.
Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
Diffs (updated)
-----
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/51010/diff/
Testing
-------
Run the newly added test.
Thanks,
anilkumar gingade
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by anilkumar gingade <ag...@pivotal.io>.
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> >
Jason, Thanks for the review...Good points...
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java, line 82
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471613#file1471613line82>
> >
> > Is this method needed?
I like to keep it so that, if future there is need for any intial settings that can be done here...One could add it in the future, but it makes it easier having the method there...
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java, line 115
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471613#file1471613line115>
> >
> > Should we limit the index here or rely on the host.getVM for which index values are valid?
> >
> > If we do want to limit it in this rule, maybe break out 1 and 3 into constants?
We can rely on host.getVM...just need to check on vm_0 used for Locator...Will make that change...
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java, line 125
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471613#file1471613line125>
> >
> > If mcast port is set... what will happen? This checks to see if it's not set.
If set it will use the MCAST port set by the test...I need to do the same change with getNode() will add that...
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java, line 103
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471614#file1471614line103>
> >
> > Should we seperate this test into two tests? One for when the new node is created in the group and one for when it is out of the group? This test is named in a way that made me think every new node was in the group and should have the index, but vm3 is outside the group and verifies it does not get an index
Good idea...I will split this...
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java, line 154
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471614#file1471614line154>
> >
> > What currently happens if we have RegionShortcut.REPLICATE? The index itself is just not created but everything else keeps on running?
I believe so..Its upto the gfsh command how we have added...
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java, line 189
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471614#file1471614line189>
> >
> > Will this dir and clusterConfigDir get cleaned up after the test is run? Should this use a TemporaryFolder rule or something else?
It uses the TemporaryFolder...yes it clears the config dir...Without this i was having problem with the second test, where it was loading the cluster config from previous test...
- anilkumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145572
-----------------------------------------------------------
On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 10:34 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by anilkumar gingade <ag...@pivotal.io>.
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java, line 189
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471614#file1471614line189>
> >
> > Will this dir and clusterConfigDir get cleaned up after the test is run? Should this use a TemporaryFolder rule or something else?
>
> anilkumar gingade wrote:
> It uses the TemporaryFolder...yes it clears the config dir...Without this i was having problem with the second test, where it was loading the cluster config from previous test...
>
> Jason Huynh wrote:
> Out of curiosity, how is the TemporaryFolder being used here? Isn't there a TemporaryFolder rule that needs to be created and used? This looks like it's just doing new File().mkdir()?
>
> anilkumar gingade wrote:
> I followed the logic from ohter tests...
>
> The file path is obtained from TemporaryFolder Rule, When the test ends the Rule deletes the dir using the path info.
>
> final String nodeDir = this.temporaryFolder.getRoot().getCanonicalPath()
> + File.separator + vmIndex;
> File workingDir = new File(nodeDir);
> workingDir.mkdirs();
>
> Jason Huynh wrote:
> Oh, I missed that part. I think temporaryFolder has a method for newFolder("") and newFile().
Good point...I will use the newFolder...
- anilkumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145572
-----------------------------------------------------------
On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 10:34 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by Jason Huynh <hu...@gmail.com>.
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java, line 189
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471614#file1471614line189>
> >
> > Will this dir and clusterConfigDir get cleaned up after the test is run? Should this use a TemporaryFolder rule or something else?
>
> anilkumar gingade wrote:
> It uses the TemporaryFolder...yes it clears the config dir...Without this i was having problem with the second test, where it was loading the cluster config from previous test...
Out of curiosity, how is the TemporaryFolder being used here? Isn't there a TemporaryFolder rule that needs to be created and used? This looks like it's just doing new File().mkdir()?
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145572
-----------------------------------------------------------
On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 10:34 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by Jason Huynh <hu...@gmail.com>.
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java, line 189
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471614#file1471614line189>
> >
> > Will this dir and clusterConfigDir get cleaned up after the test is run? Should this use a TemporaryFolder rule or something else?
>
> anilkumar gingade wrote:
> It uses the TemporaryFolder...yes it clears the config dir...Without this i was having problem with the second test, where it was loading the cluster config from previous test...
>
> Jason Huynh wrote:
> Out of curiosity, how is the TemporaryFolder being used here? Isn't there a TemporaryFolder rule that needs to be created and used? This looks like it's just doing new File().mkdir()?
>
> anilkumar gingade wrote:
> I followed the logic from ohter tests...
>
> The file path is obtained from TemporaryFolder Rule, When the test ends the Rule deletes the dir using the path info.
>
> final String nodeDir = this.temporaryFolder.getRoot().getCanonicalPath()
> + File.separator + vmIndex;
> File workingDir = new File(nodeDir);
> workingDir.mkdirs();
Oh, I missed that part. I think temporaryFolder has a method for newFolder("") and newFile().
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145572
-----------------------------------------------------------
On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 10:34 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by anilkumar gingade <ag...@pivotal.io>.
> On Aug. 11, 2016, 11:16 p.m., Jason Huynh wrote:
> > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java, line 189
> > <https://reviews.apache.org/r/51010/diff/1/?file=1471614#file1471614line189>
> >
> > Will this dir and clusterConfigDir get cleaned up after the test is run? Should this use a TemporaryFolder rule or something else?
>
> anilkumar gingade wrote:
> It uses the TemporaryFolder...yes it clears the config dir...Without this i was having problem with the second test, where it was loading the cluster config from previous test...
>
> Jason Huynh wrote:
> Out of curiosity, how is the TemporaryFolder being used here? Isn't there a TemporaryFolder rule that needs to be created and used? This looks like it's just doing new File().mkdir()?
I followed the logic from ohter tests...
The file path is obtained from TemporaryFolder Rule, When the test ends the Rule deletes the dir using the path info.
final String nodeDir = this.temporaryFolder.getRoot().getCanonicalPath()
+ File.separator + vmIndex;
File workingDir = new File(nodeDir);
workingDir.mkdirs();
- anilkumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145572
-----------------------------------------------------------
On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 10:34 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 51010: GEODE-11 Test for Cluster Configuration
changes for Lucene index.
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51010/#review145572
-----------------------------------------------------------
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 82)
<https://reviews.apache.org/r/51010/#comment211869>
Is this method needed?
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 115)
<https://reviews.apache.org/r/51010/#comment211874>
Should we limit the index here or rely on the host.getVM for which index values are valid?
If we do want to limit it in this rule, maybe break out 1 and 3 into constants?
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 125)
<https://reviews.apache.org/r/51010/#comment211875>
If mcast port is set... what will happen? This checks to see if it's not set.
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 129)
<https://reviews.apache.org/r/51010/#comment211878>
Should this be a SerializableRunnable?
geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java (line 156)
<https://reviews.apache.org/r/51010/#comment211877>
Should this be a SerializableRunnable? It doesn't seem to matter what happens in this method, it will always return true and we are not actually using the return value anyways
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java (line 83)
<https://reviews.apache.org/r/51010/#comment211879>
I think these methods are self documenting, probably can remove the comments
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java (line 103)
<https://reviews.apache.org/r/51010/#comment211864>
Should we seperate this test into two tests? One for when the new node is created in the group and one for when it is out of the group? This test is named in a way that made me think every new node was in the group and should have the index, but vm3 is outside the group and verifies it does not get an index
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java (line 154)
<https://reviews.apache.org/r/51010/#comment211865>
What currently happens if we have RegionShortcut.REPLICATE? The index itself is just not created but everything else keeps on running?
geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java (line 189)
<https://reviews.apache.org/r/51010/#comment211863>
Will this dir and clusterConfigDir get cleaned up after the test is run? Should this use a TemporaryFolder rule or something else?
- Jason Huynh
On Aug. 11, 2016, 10:34 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51010/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2016, 10:34 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Jason Huynh, Kirk Lund, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Added test to validate Cluster configuration support for Lucene indexes.
>
> Added a new Rule to create Distributed System with test/custom configuration...The "LocatorServerConfigurationRule" makes it easier to create a Locator or cluster nodes with required configuration.
>
>
> Diffs
> -----
>
> geode-core/src/test/java/com/gemstone/gemfire/test/dunit/rules/LocatorServerConfigurationRule.java PRE-CREATION
> geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/51010/diff/
>
>
> Testing
> -------
>
> Run the newly added test.
>
>
> Thanks,
>
> anilkumar gingade
>
>