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