----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16322/#review30627 -----------------------------------------------------------
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/#comment58646> Seems like this comment is can be removed. src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58649> Please add the license header. src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58647> Please remove blank lines src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58648> Remove empty space. src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58650> Remove commented lines. src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58651> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58652> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58653> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58654> 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/#comment58655> Please replace "magic number" with a constant. src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58656> Do we really need the comment here? src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58657> Do we really need the comment here? src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58659> Do we really need the comment here? src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58658> Do we really need the comment here? src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58660> Do we really need the comment here? src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58661> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58662> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58663> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58664> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58665> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58666> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58667> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58669> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58668> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58694> 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/#comment58670> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58671> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58672> 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/#comment58674> 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/#comment58675> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58676> 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/#comment58677> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58679> 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/#comment58680> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58682> 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/#comment58684> Remove blank line src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58685> Remove blank line src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58686> Remove blank line src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58687> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58688> Remove comment src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58690> Seems like this return is not needed. src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java <https://reviews.apache.org/r/16322/#comment58695> Please remove the commented block. src/org/waveprotocol/box/server/robots/agent/welcome/WelcomeRobot.java <https://reviews.apache.org/r/16322/#comment58696> Good catch! src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java <https://reviews.apache.org/r/16322/#comment58697> Please remove the trailing space. src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java <https://reviews.apache.org/r/16322/#comment58698> Please add your name to todo. src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java <https://reviews.apache.org/r/16322/#comment58701> 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/#comment58702> Please add //TODO here and your name. src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java <https://reviews.apache.org/r/16322/#comment58703> 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/#comment58704> Do we really need the comment? src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58706> Please remove trailing space. src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58705> 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/#comment58709> Is it XXX or just a comment? src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58710> Is it XXX or just a comment? src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58711> Do we really need this comment? src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58712> Remove commented code. src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58713> Remove commented code src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58714> Remove commented code. src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58715> Remove commented code. src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58716> Please make here proper todo, or is it a comment? src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java <https://reviews.apache.org/r/16322/#comment58718> Please remove println - Yuri Zelikov On Dec. 17, 2013, 4:08 p.m., Frank R. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16322/ > ----------------------------------------------------------- > > (Updated Dec. 17, 2013, 4:08 p.m.) > > > Review request for wave, Ali Lown, Vicente J. Ruiz Jurado, and Yuri Zelikov. > > > 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/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 > > 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. > >
