----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46065/#review128306 -----------------------------------------------------------
Looks good! I had a couple of minor comments, see below. geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexForPartitionedRegion.java (line 56) <https://reviews.apache.org/r/46065/#comment191732> It's a bit redundant to have both a java assert and an IllegalStateException. Since this class is only used for partitioned regions, it's probably not necassary to check if it's a PR at all. Just assume it is. If there is a logic error we'll get an NPE anyway. geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplJUnitTest.java (line 228) <https://reviews.apache.org/r/46065/#comment191733> I would probably not put assertions about the message contents in the test - messages are not required to be stable. That said, the message should be "*are* not yet implemented" or maybe "are not supported." - Dan Smith On April 12, 2016, 12:18 a.m., xiaojian zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46065/ > ----------------------------------------------------------- > > (Updated April 12, 2016, 12:18 a.m.) > > > Review request for geode and Dan Smith. > > > Bugs: geode-1212 > https://issues.apache.org/jira/browse/geode-1212 > > > Repository: geode > > > Description > ------- > > The objective is: > 1) If tried to create LuceneIndex on a replicated region, should throw > UnsupportedOperationException > 2) No API to mislead user to create index on replicated region > 3) add test case to verify > > I did not remove the class LuceneIndexForReplicatedRegion, since it has > implemented to throw UnsupportedOperationException for all its methods. This > is also an option. > > > Diffs > ----- > > > geode-lucene/src/main/java/com/gemstone/gemfire/cache/lucene/internal/LuceneIndexForPartitionedRegion.java > 7002567 > > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneQueryImplJUnitTest.java > 3975ac3 > > geode-lucene/src/test/java/com/gemstone/gemfire/cache/lucene/internal/LuceneServiceImplJUnitTest.java > 159fd46 > > Diff: https://reviews.apache.org/r/46065/diff/ > > > Testing > ------- > > > Thanks, > > xiaojian zhou > >
