Hi Yuri Thanks for your comments.
I've updated the files, and made some fixes as well. - Empty wave will be listed in search results - solr-bot will highlight results properly when the highlighting is at the end of a snippet Shall I submit a new patch based on my first one, or on the same base, i.e. 985cd57fbc08ca8dc9f332929d6d3c06399d20ea? Frank On Thu, Dec 19, 2013 at 3:47 AM, Yuri Zelikov <vega...@gmail.com> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16322/ > > I did a first pass - this is really a great Job! Thanks! > Please find some comments below. > > > > src/org/waveprotocol/box/server/SearchModule.java<https://reviews.apache.org/r/16322/diff/2/?file=398899#file398899line79> > (Diff > revision 2) > > public class SearchModule extends AbstractModule { > > 78 > > * required by > org.waveprotocol.box.server.ServerMain.initializeSearch(Injector, WaveBus) > > Seems like this comment is can be removed. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line1> > (Diff > revision 2) > > 1 > > package org.waveprotocol.box.server.robots.agent.search; > > Please add the license header. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line3> > (Diff > revision 2) > > 3 > > Please remove blank lines > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line55> > (Diff > revision 2) > > 55 > > * > > Remove empty space. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line70> > (Diff > revision 2) > > 70 > > // private static final Logger LOG = > > Remove commented lines. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line121> > (Diff > revision 2) > > 121 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line127> > (Diff > revision 2) > > 127 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line135> > (Diff > revision 2) > > 135 > > // String modifiedBy = event.getModifiedBy(); > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line138> > (Diff > revision 2) > > 138 > > * http://wave-protocol.googlecode.com/hg/spec/conversation/convspec.html > > Do we really need here the reference to the doc? > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line150> > (Diff > revision 2) > > 150 > > if (now - timestamp <= 3000) { > > Please replace "magic number" with a constant. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line163> > (Diff > revision 2) > > 163 > > * removed "start == range.getEnd()" to allow query at any line (even > if > > Do we really need the comment here? > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line172> > (Diff > revision 2) > > 172 > > * XXX the commented code does work as expected > > Do we really need the comment here? > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line182> > (Diff > revision 2) > > 182 > > // * XXX only listens to the creator > > Do we really need the comment here? > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line187> > (Diff > revision 2) > > 187 > > * XXX the creator will be aware of the query > > Do we really need the comment here? > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line199> > (Diff > revision 2) > > 199 > > * XXX if only the last line is accepted, we can quit the loop > > Do we really need the comment here? > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line211> > (Diff > revision 2) > > 211 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line217> > (Diff > revision 2) > > 217 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line223> > (Diff > revision 2) > > 223 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line235> > (Diff > revision 2) > > 235 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line241> > (Diff > revision 2) > > 241 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line247> > (Diff > revision 2) > > 247 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line258> > (Diff > revision 2) > > 258 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line264> > (Diff > revision 2) > > 264 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line270> > (Diff > revision 2) > > 270 > > // TODO Auto-generated method stub > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line274> > (Diff > revision 2) > > 274 > > private void search(String query, String creator, Blip outputBlip) { > > The method is too long. Consider refactoring it by extracting different > logic into separate methods, like input parsing, sending request to Solr, > Solr response processing, creating output etc.. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line276> > (Diff > revision 2) > > 276 > > // if (query.length() <= 0) { > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line289> > (Diff > revision 2) > > 289 > > // StringBuilder messageBuilder = new StringBuilder(); > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line293> > (Diff > revision 2) > > 293 > > * XXX will it be better to replace lucene with edismax? > > Should this XXX be addressed? Or remove the comment otherwise. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line295> > (Diff > revision 2) > > 295 > > String fq; > > Can we have a more descriptive name for the "fq"? > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line327> > (Diff > revision 2) > > 327 > > // return "Failed to execute query: " + query; > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line344> > (Diff > revision 2) > > 344 > > * TODO insert at the beginning the number of waves found > > Should the todo be addressed? If so, please add your name near todo, like: > TODO (yuri): insert at the beginning the number of waves found. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line346> > (Diff > revision 2) > > 346 > > // outputBlip.append("Found: " + > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line358> > (Diff > revision 2) > > 358 > > * TODO > > Please address the todo or add your name and some description of what meed > to be done. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line372> > (Diff > revision 2) > > 372 > > // String result = > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line406> > (Diff > revision 2) > > 406 > > .annotate("style/backgroundColor", "rgb(255,255,0)"); > > Consider using constants instead of > style/backgroundColor > rgb(255,255,0) > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line412> > (Diff > revision 2) > > 412 > > Remove blank line > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line416> > (Diff > revision 2) > > 416 > > Remove blank line > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line428> > (Diff > revision 2) > > 428 > > Remove blank line > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line431> > (Diff > revision 2) > > 431 > > // return "Failed to execute query: " + query; > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line438> > (Diff > revision 2) > > 438 > > // return messageBuilder.toString(); > > Remove comment > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line439> > (Diff > revision 2) > > 439 > > return; > > Seems like this return is not needed. > > > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line442> > (Diff > revision 2) > > 442 > > // private String update(String modifiedBy) { > > Please remove the commented block. > > > > src/org/waveprotocol/box/server/robots/agent/welcome/WelcomeRobot.java<https://reviews.apache.org/r/16322/diff/2/?file=398902#file398902line59> > (Diff > revision 2) 59 > > private static final Logger LOG = > Logger.getLogger(PasswordRobot.class.getName()); > > 59 > > private static final Logger LOG = > Logger.getLogger(WelcomeRobot.class.getName()); > > Good catch! > > > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398903#file398903line54> > (Diff > revision 2) > > 54 > > * > > Please remove the trailing space. > > > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398903#file398903line78> > (Diff > revision 2) > > 78 > > * TODO make it configurable > > Please add your name to todo. > > > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398903#file398903line94> > (Diff > revision 2) > > 94 > > /*- > > Please remove the trailing spaces and add your names to XXX > > > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398903#file398903line135> > (Diff > revision 2) > > 135 > > // Maybe should be changed in case other folders in addition to 'inbox' > are > > Please add //TODO here and your name. > > > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398903#file398903line139> > (Diff > revision 2) > > 139 > > Multimap<WaveId, WaveletId> currentUserWavesView = HashMultimap.create(); > > Looks like we can try to refactor some common code from SolrRobot.java into > a separate helper class and use it here. > > > > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398903#file398903line186> > (Diff > revision 2) > > 186 > > * TODO > > Do we really need the comment? > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line74> > (Diff > revision 2) > > 74 > > * > > Please remove trailing space. > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line78> > (Diff > revision 2) > > 78 > > public static String readText(ReadableBlipData doc) { > > Seems like this code is duplicated from other class, can we refactor it > into a separate module and re-use in both classes? > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line145> > (Diff > revision 2) > > 145 > > * XXX ignored. See waveletCommitted(WaveletName, HashedVersion) > > Is it XXX or just a comment? > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line154> > (Diff > revision 2) > > 154 > > * XXX ignored. See waveletCommitted(WaveletName, HashedVersion) > > Is it XXX or just a comment? > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line200> > (Diff > revision 2) > > 200 > > * update solr index > > Do we really need this comment? > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line205> > (Diff > revision 2) > > 205 > > // postMethod.setRequestHeader("Content-Type", "application/json"); > > Remove commented code. > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line220> > (Diff > revision 2) > > 220 > > // String text = Snippets.collateTextForWavelet(wavelet); > > Remove commented code > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line260> > (Diff > revision 2) > > 260 > > // LOG.fine(postMethod.getResponseBodyAsString()); > > Remove commented code. > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line274> > (Diff > revision 2) > > 274 > > * commented out for optimization, see waveletCommitted(WaveletName, > HashedVersion) > > Remove commented code. > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line285> > (Diff > revision 2) > > 285 > > * XXX don't update index here (on current thread) to prevent lock > > Please make here proper todo, or is it a comment? > > > > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java<https://reviews.apache.org/r/16322/diff/2/?file=398904#file398904line294> > (Diff > revision 2) > > 294 > > System.out.println("commit " + version + " " + > waveletData.getVersion()); > > Please remove println > > > - Yuri Zelikov > > On December 17th, 2013, 4:08 p.m. UTC, Frank R. wrote: > Review request for wave, Ali Lown, Vicente J. Ruiz Jurado, and Yuri > Zelikov. > By Frank R.. > > *Updated Dec. 17, 2013, 4:08 p.m.* > *Repository: * wave > Description > > For details (issues and commits):https://github.com/renfeng/wave > > 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 > > Diffs > > - run-export.sh (d2cddb7) > - run-import.sh (45fff8a) > - server.config.example (19ba8b2) > - 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/SolrSearchProviderImpl.java > (PRE-CREATION) > - src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java > (PRE-CREATION) > > View Diff <https://reviews.apache.org/r/16322/diff/> >
