You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@lucene.apache.org by LucVL <gi...@git.apache.org> on 2015/10/05 13:32:38 UTC

[GitHub] lucene-solr pull request: SOLR-8050: Test case demonstrating the b...

GitHub user LucVL opened a pull request:

    https://github.com/apache/lucene-solr/pull/202

    SOLR-8050: Test case demonstrating the bug

    To run just this testcase, use:
    ```sh
    ant test -Dtests.class=org.apache.solr.update.processor.AtomicUpdatesTest -Dtests.method=testMultipleTDateValues
    ```

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/LucVL/lucene-solr SOLR-8050

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/lucene-solr/pull/202.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #202
    
----
commit bb7b239eb25a8826e9767edc52e970a8b2aab405
Author: Luc Vanlerberghe <lu...@bvdinfo.com>
Date:   2015-10-05T09:58:56Z

    Test case demonstrating the bug

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr issue #202: SOLR-10703 Add prepare and finish in DocTrasformer

Posted by dsmiley <gi...@git.apache.org>.

Github user dsmiley commented on the issue:

    https://github.com/apache/lucene-solr/pull/202
  
    Maybe this should use Solr's `IOUtils.closeQuietly`?  It's tempting to use Lucene's `IOUtils.closeWhileHandlingException` since it'll save us from even looping here, although that one doesn't log the exception; it's swallowed completely :-/


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr pull request #202: SOLR-10703 Add prepare and finish in DocTrasf...

Posted by diegoceccarelli <gi...@git.apache.org>.

Github user diegoceccarelli commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/202#discussion_r120700095
  
    --- Diff: solr/core/src/java/org/apache/solr/response/transform/DocTransformers.java ---
    @@ -88,4 +89,11 @@ public boolean needsSolrIndexSearcher() {
         return false;
       }
     
    +  @Override
    +  public void finish() {
    +    for( DocTransformer a : children ) {
    +      a.finish();
    --- End diff --
    
    yes? I think that if a transformer throws an exception solr request should fail (you can still try and catch the exception inside the transformer if you want to obtain 'silent' behaviour) what do you think? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr pull request #202: SOLR-10703 Add prepare and finish in DocTrasf...

Posted by diegoceccarelli <gi...@git.apache.org>.

Github user diegoceccarelli commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/202#discussion_r120698022
  
    --- Diff: solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java ---
    @@ -296,13 +297,13 @@ public void process(ResponseBuilder rb) throws IOException
              if (null == resultContext) {
                // either first pass, or we've re-opened searcher - either way now we setContext
                resultContext = new RTGResultContext(rsp.getReturnFields(), searcherInfo.getSearcher(), req);
    -           transformer.setContext(resultContext);
    +           transformer.prepare(resultContext);
              }
              transformer.transform(doc, docid, 0);
            }
            docList.add(doc);
          }
    -
    +     if ( null != transformer) transformer.finish();
    --- End diff --
    
    I'll take a look, thank you


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr pull request #202: SOLR-10703 Add prepare and finish in DocTrasf...

Posted by diegoceccarelli <gi...@git.apache.org>.

Github user diegoceccarelli commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/202#discussion_r120699342
  
    --- Diff: solr/core/src/java/org/apache/solr/response/transform/DocTransformer.java ---
    @@ -41,12 +41,26 @@
        */
       public abstract String getName();
     
    +
    +  /**
    +   * This is called before {@link #transform} to provide context for any subsequent transformations.
    +   *
    +   * @param context The {@link ResultContext} stores information about how the documents were produced.
    +   * @see #needsSolrIndexSearcher
    +   */
    +  public void prepare( ResultContext context ) {
    --- End diff --
    
    if you want to do something different than setting the context (see LTRFeatureLoggerTransformerFactory for example), the name is misleading. we could keep the setContext and just add the `prepare` method. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr pull request #202: SOLR-10703 Add prepare and finish in DocTrasf...

Posted by m-khl <gi...@git.apache.org>.

Github user m-khl commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/202#discussion_r119992969
  
    --- Diff: solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java ---
    @@ -1210,7 +1211,7 @@ public SolrQueryRequest getRequest() {
         }
         
         /** @returns null */
    -    public Iterator<SolrDocument> getProcessedDocuments() {
    --- End diff --
    
    not clear why 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr pull request #202: SOLR-10703 DocTransformer implements Closeabl...

Posted by tflobbe <gi...@git.apache.org>.

Github user tflobbe commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/202#discussion_r137335378
  
    --- Diff: solr/core/src/test/org/apache/solr/response/TestCustomDocTransformer.java ---
    @@ -125,4 +171,86 @@ public static String getAsString(String field, SolrDocument doc) {
         }
         return null;
       }
    +
    +
    +  public static class CustomFinishTransformerFactory extends TransformerFactory {
    +
    +    static CustomFinishTransformer finishTrasformer = new CustomFinishTransformer();
    +
    +    @Override
    +    public DocTransformer create(String field, SolrParams params, SolrQueryRequest req) {
    +      return finishTrasformer;
    +    }
    +  }
    +
    +
    +  public static class CustomFinishTransformer extends DocTransformer {
    +    int counter;
    +
    +    public CustomFinishTransformer() {
    +    }
    +
    +    @Override
    +    public void setContext(ResultContext context){
    +      super.setContext(context);
    +      counter = 0;
    +    }
    +
    +    @Override
    +    public String getName() {
    +      return "customFinish";
    +    }
    +
    +    @Override
    +    public void transform(SolrDocument doc, int docid) throws IOException {
    +      counter++;
    --- End diff --
    
    Suggestion. Throw exception if close() was already called in this request


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr pull request #202: SOLR-10703 DocTransformer implements Closeabl...

Posted by tflobbe <gi...@git.apache.org>.

Github user tflobbe commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/202#discussion_r137335651
  
    --- Diff: solr/core/src/test/org/apache/solr/response/TestCustomDocTransformer.java ---
    @@ -68,6 +69,51 @@ public void testCustomTransformer() throws Exception {
             "//str[.='xx#title_2#']",
             "//str[.='xx#title_3#']");
       }
    +
    +  @Test
    +  public void testFinishCallInCustomFinishTransformer() throws Exception {
    +    // Build a simple index
    +    int max = 10;
    +    for(int i=0; i<max; i++) {
    +      SolrInputDocument sdoc = new SolrInputDocument();
    +      sdoc.addField("id", i);
    +      sdoc.addField("subject", "xx");
    +      sdoc.addField("title", "title_"+i);
    +      updateJ(jsonAdd(sdoc), null);
    +    }
    +    assertU(commit());
    +    assertQ(req("q", "*:*"), "//*[@numFound='" + max + "']");
    +
    +    assertQ( req(
    +        "q", "*:*",
    +        "fl", "id,[customFinish]",
    +        "rows", String.valueOf(max)
    +        ),
    +        // Check that the concatenated fields make it in the results
    +        "//*[@numFound='" + max + "']");
    +
    +    // finish() will double the number of documents
    +    assertEquals(max*2, CustomFinishTransformerFactory.finishTrasformer.counter);
    +    CustomFinishTransformerFactory.finishTrasformer.counter = 0;
    +
    +    // test binary writer
    +    h.query(req(
    +        "q", "*:*",
    +        "fl", "id,[customFinish]",
    +        "rows", String.valueOf(max),
    +        "wt", "javabin"
    +    ));
    +    assertEquals(max*2, CustomFinishTransformerFactory.finishTrasformer.counter);
    +
    +    // test that a transformer that throws an exception doesn't affect the other transformers
    +    h.query(req(
    +        "q", "*:*",
    +        "fl", "id,[exceptionFinish],[customFinish]",
    +        "rows", String.valueOf(max),
    +        "wt", "javabin"
    +    ));
    +    assertEquals(max*2, CustomFinishTransformerFactory.finishTrasformer.counter);
    --- End diff --
    
    Suggestion: Assert that the exception was actually thrown, and add a "ignoreException" to not pollute the logs


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr pull request #202: SOLR-10703 DocTransformer implements Closeabl...

Posted by diegoceccarelli <gi...@git.apache.org>.

Github user diegoceccarelli commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/202#discussion_r139309859
  
    --- Diff: solr/solrj/src/java/org/apache/solr/common/util/IOUtils.java ---
    @@ -31,7 +32,7 @@ public static void closeQuietly(Closeable closeable) {
             closeable.close();
           }
         } catch (Exception e) {
    -      LOG.error("Error while closing", e);
    +      SolrException.log(LOG, "Error while closing "+closeable.getClass());
    --- End diff --
    
    I had to modify the logging here in order to have `ignoreException` working in the tests. 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

[GitHub] lucene-solr issue #202: SOLR-10703 Add prepare and finish in DocTrasformer

Posted by diegoceccarelli <gi...@git.apache.org>.

Github user diegoceccarelli commented on the issue:

    https://github.com/apache/lucene-solr/pull/202
  
    @m-khl  I updated the patch, highlights: 
       1. I removed `prepare` and relied on the old `setContext()`;
       2. now a DocTransformer implements Closeable and provides the method `close()`
       3. If a transformer fails it doesn't affect the other trasformers (+ Test)
    
    TODO:
    I didn't find a way to check that DocTrasformer::close is always called in the tests, any idea? 
    
    The `close()` is called from `DocsStreamer` so RealTimeGetRequest component should not be affected by this change, do you think it should? 
    
    
    
    
    
    
    minor: I think we should rename the Jira item in something like `Make DocTransformer Closeable` or something like that


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org