----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16322/#review30918 -----------------------------------------------------------
Some more comments. You are doing a great job but it looks to me that the change that you suggest is really huge. Maybe it is better to split it into two parts? I think you can handle just the full text search in first part and add the Solr robot in second. What you think? src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment59140> Consider using Lists.newArrayList() src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java <https://reviews.apache.org/r/16322/#comment59141> Is this line too long? Should be under 100 chars per line. src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java <https://reviews.apache.org/r/16322/#comment59143> Isn't SimpleSearchProviderImpl already implements SearchProvider? src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java <https://reviews.apache.org/r/16322/#comment59142> I think this matches function is already defines in other place. src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java <https://reviews.apache.org/r/16322/#comment59144> 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. src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment59146> I guess you copied this line form other place, anyway please replce Yuri Z. with your name :) src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment59147> I think Ali already mentioned - use Snippets.collateTextForDocuments instead of duplicating the code. src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment59148> 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. src/org/waveprotocol/box/server/waveserver/WaveDigester.java <https://reviews.apache.org/r/16322/#comment59149> If you are at it, I think it would be better to enforce this precondition with Preconditions.checkState() - Yuri Zelikov On Dec. 23, 2013, 3: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, 3: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. > >
