> On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java, > > line 77 > > <https://reviews.apache.org/r/16322/diff/10/?file=402363#file402363line77> > > > > Is this line too long? Should be under 100 chars per line.
It's 99. > On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java, > > line 60 > > <https://reviews.apache.org/r/16322/diff/10/?file=402364#file402364line60> > > > > Isn't SimpleSearchProviderImpl already implements SearchProvider? You're right. It's intended to have SolrSearchProvider as a direct implementing class for SearchProvider. It's only for reusing some code in SimpleSearchProviderImpl to have "extends SimpleSearchProviderImpl". I would rather suggest you to consider refactoring SimpleSearchProviderImpl by moving the shared code somewhere else. > On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java, > > line 120 > > <https://reviews.apache.org/r/16322/diff/10/?file=402364#file402364line120> > > > > I think this matches function is already defines in other place. I'm afraid not. You can verify that by searching through the code. > On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java, line 170 > > <https://reviews.apache.org/r/16322/diff/10/?file=402361#file402361line170> > > > > Consider using Lists.newArrayList() What would be the difference? @GwtCompatible(serializable = true) public static <E> ArrayList<E> newArrayList() { return new ArrayList<E>(); } > On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java, > > line 285 > > <https://reviews.apache.org/r/16322/diff/10/?file=402364#file402364line285> > > > > Seems like the only modification is to skip results ordering - that > > will make the results returned by the search provider unstable. If you > > don't need ordering by queryParams, then it is still better to order by LMT > > and wave id which can be achived by passing Collections.emptySet() as the > > last argument to the original computeSearchResult() method. Ordering is taken care of by Solr - by relevance by default. See http://wiki.apache.org/solr/CommonQueryParameters#sort Currently, only the default ordering is implemented. Does it worth anything to pass in an empty set and invoke sortedCopy? > On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java, line 83 > > <https://reviews.apache.org/r/16322/diff/10/?file=402365#file402365line83> > > > > I guess you copied this line form other place, anyway please replce > > Yuri Z. with your name :) May I leave it as it is? I just borrowed the idea from you (Yuri) without fully understanding what it was. > On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java, line > > 105 > > <https://reviews.apache.org/r/16322/diff/10/?file=402365#file402365line105> > > > > I think Ali already mentioned - use Snippets.collateTextForDocuments > > instead of duplicating the code. I was well aware of this hack as you can read in the comments here /*- * copied with modifications from * org.waveprotocol.box.common.Snippets.collateTextForOps(Iterable<DocOp>) * * replaced white space character with new line */ and here /* * (regression alert) cannot reuse Snippets because it trims the * content. */ // Iterable<DocOp> docs = Arrays.asList((DocOp) // document.getContent().asOperation()); // String text = Snippets.collateTextForOps(docs); It was the single newline that was intended to be displayed as it is in search results by solr-bot with highlighting, and helped me to filter user blips. Unfortunately, trimmed by the method of Snippets. Let me try it again since both things changed. - perhaps a white space is more readable than new line in search result snippets - I had a better way to distinguish user blips. > On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java, line > > 456 > > <https://reviews.apache.org/r/16322/diff/10/?file=402365#file402365line456> > > > > When i wrote the remakeIndex for AbstractWaveIndexer, my intention was > > to allow remaking the index to migrate form memory search to lucene. i.e. > > it should be triggered automatically if the _indexes folder is empty. So > > you don't need to clean up the folder here. > > The basic idea here is that you load all wavelets in the old fashion of > > meory search and then index the waves one by one. And the actual per wave > > indexing should happen in the processWavelet hook. It seems like you can > > move the Solr indexing logic to processWavelet. In fact I think that > > SolrWaveIndexer should look identical to LuceneWaveIndexerImpl, as indexing > > a new wave is identical to the even when a user creates a new wave. I agree with you on one condition - someone will have written an admin guide to tell users when and how to clean up Solr index data store. I'd consider Solr index more like a temporary data store, or cache. It should be cleaned from time to time to keep its data refreshed. There are problems with Lucene to keep its index store valid. From my experience of running Wave in a box, I need to manually delete _index occasionally when I saw some error unrecoverable by just restarting the server. > On Dec. 28, 2013, 3:01 a.m., Yuri Zelikov wrote: > > src/org/waveprotocol/box/server/waveserver/WaveDigester.java, line 92 > > <https://reviews.apache.org/r/16322/diff/10/?file=402366#file402366line92> > > > > If you are at it, I think it would be better to enforce this > > precondition with Preconditions.checkState() It's legacy from others. What I did was simply refactoring something into the public method for reuse with minimum changes. org.waveprotocol.box.server.waveserver.WaveDigester.build(ParticipantId, WaveViewData) - Frank ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16322/#review30918 ----------------------------------------------------------- On Dec. 23, 2013, 11:53 p.m., Frank R. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16322/ > ----------------------------------------------------------- > > (Updated Dec. 23, 2013, 11:53 p.m.) > > > Review request for wave, Ali Lown, Vicente J. Ruiz Jurado, and Yuri Zelikov. > > > Bugs: WAVE-311 > https://issues.apache.org/jira/browse/WAVE-311 > > > Repository: wave > > > Description > ------- > > For details (issues and commits): > https://github.com/renfeng/wave > > > Diffs > ----- > > run-export.sh d2cddb7 > run-import.sh 45fff8a > server.config.example 19ba8b2 > src/com/google/wave/api/Annotation.java b55f778 > src/org/waveprotocol/box/server/SearchModule.java 2de0ef9 > src/org/waveprotocol/box/server/ServerMain.java b50454d > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java > PRE-CREATION > src/org/waveprotocol/box/server/robots/agent/welcome/WelcomeRobot.java > 2735940 > src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java > ee7093f > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java > PRE-CREATION > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java > PRE-CREATION > src/org/waveprotocol/box/server/waveserver/WaveDigester.java b103bbb > > Diff: https://reviews.apache.org/r/16322/diff/ > > > Testing > ------- > > tests on search box > > * in:inbox > * (empty) for all, including waves shared in the domain > * with:@ > * (free texts) > > tests on solr-bot > > * single word > * phrase (quoted with double quotation marks) > * syntax applicable to search box > > > Thanks, > > Frank R. > >
