incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Frank R." <renfeng...@gmail.com>
Subject Re: Review Request 16322: full text search (with known issue of solr-bot output highlight)
Date Thu, 19 Dec 2013 01:04:42 GMT
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 <vega113@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/>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message