> On Dec. 27, 2013, 7:01 p.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 :) > > Frank R. wrote: > May I leave it as it is? I just borrowed the idea from you (Yuri) without > fully understanding what it was.
The executor is need to to execute the future when it is ready. Using singleThreaded executor is the most safe, but not most efficient option. It is better to provide a setting in the server.config so the admin can decide on the size of the thread pool for the executor. You can take a look on how it was implemented for some other executors at ServerMain lines 155-164 and then follow from there. > On Dec. 27, 2013, 7:01 p.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. > > Frank R. wrote: > 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? Yeah, you are right. I had to make the method more modular, i.e. extracting the sorting into a separate method that subclasses would be able to ovveride: protected List<WaveViewData> sortResults(Map<TokenQueryType, Set<String>> queryParams, Map<WaveId, WaveViewData> results) { return QueryHelper.computeSorter(queryParams).sortedCopy(results.values()); } and then calling it from computeSearchResult: searchResultslist = sortResults(queryParams, results) .subList(startAt, endAt); Would you like me to make this change or would you prefer to make it by yourself? > On Dec. 27, 2013, 7:01 p.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. > > Frank R. wrote: > 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. I see. Can you maybe overload this method in Snippets to accept additional parameter that will tell it which which separator to use and then make the original method to call the new one with space? We really need to avoid code duplication much as possible. > On Dec. 27, 2013, 7:01 p.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? > > Frank R. wrote: > 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. Ok, would you like me to make the change and then update this patch accordingly? > On Dec. 27, 2013, 7:01 p.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() > > Frank R. wrote: > What would be the difference? > > @GwtCompatible(serializable = true) > public static <E> ArrayList<E> newArrayList() { > return new ArrayList<E>(); > } > It's shorter :) as you don't need to repeat yourself by specifying the generic type of the list. It is also always advised to use static factory methods instead of constructors. - Yuri ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16322/#review30918 ----------------------------------------------------------- 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. > >
