Solr
  1. Solr
  2. SOLR-1286

DIH: The commit parameter is always defaulting to "true" even if "false" is explicitly passed in.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4
    • Labels:
      None

      Description

      I've tried running full and delta imports with commit=false so that the autoCommit will manage all commits to the index. However setting commit=false doesn't have any effect:
      curl 'http://localhost:8080/solr/dataimporter?command=full-import&commit=false'

      1. SOLR-1286.patch
        5 kB
        Jay Hill
      2. SOLR-1286.patch
        5 kB
        Noble Paul
      3. SOLR-1286.patch
        3 kB
        Noble Paul
      4. SOLR-1286.patch
        3 kB
        Noble Paul
      5. SOLR-1286.patch
        2 kB
        Noble Paul
      6. SOLR-1286.patch
        1 kB
        Jay Hill

        Activity

        Hide
        Jay Hill added a comment -

        In DocBuilder.java added a check in two places to see if the commit para is false before doing the commit.

        Show
        Jay Hill added a comment - In DocBuilder.java added a check in two places to see if the commit para is false before doing the commit.
        Hide
        Erik Hatcher added a comment -

        I was about to commit this Jay - looks reasonable. But, the DocBuilder#commit method sets the status of indexing completion, which won't get called as you've patched. Looks like some slight refactoring of when that status is logged is warranted. Also the message "Full Import completed successfully" probably should just say "Import completed successfully" since this could be either delta or full import.

        Wanna take another round of tinkering, Jay?

        A unit test that tests whether commit is called or executed would be nice, to show the bug and the fix working - but this is straightforward enough that I'd commit as-is. But unit tests: +1

        Show
        Erik Hatcher added a comment - I was about to commit this Jay - looks reasonable. But, the DocBuilder#commit method sets the status of indexing completion, which won't get called as you've patched. Looks like some slight refactoring of when that status is logged is warranted. Also the message "Full Import completed successfully" probably should just say "Import completed successfully" since this could be either delta or full import. Wanna take another round of tinkering, Jay? A unit test that tests whether commit is called or executed would be nice, to show the bug and the fix working - but this is straightforward enough that I'd commit as-is. But unit tests: +1
        Hide
        Erik Hatcher added a comment -

        Wow, you're fast Noble! Looks better. But the patch looks like optimize won't occur unless commit=true is set. What if only optimize=true? Granted, a commit occurs implicitly during an optimize, but seems like that should be outside of the if on commit=true.

        Show
        Erik Hatcher added a comment - Wow, you're fast Noble! Looks better. But the patch looks like optimize won't occur unless commit=true is set. What if only optimize=true? Granted, a commit occurs implicitly during an optimize, but seems like that should be outside of the if on commit=true.
        Hide
        Noble Paul added a comment -

        if somebody sets optimize=true. It should implicitly set commit=true . makes sense?

        Show
        Noble Paul added a comment - if somebody sets optimize=true. It should implicitly set commit=true . makes sense?
        Hide
        Erik Hatcher added a comment - - edited

        Noble - looks good!

        Jay - give it a try and comment back. When you say it's good, I'll commit.

        unit tests, anyone?

        Show
        Erik Hatcher added a comment - - edited Noble - looks good! Jay - give it a try and comment back. When you say it's good, I'll commit. unit tests, anyone?
        Hide
        Noble Paul added a comment -

        with JUnit

        Show
        Noble Paul added a comment - with JUnit
        Hide
        Noble Paul added a comment -

        the last patch did not have the tests

        Show
        Noble Paul added a comment - the last patch did not have the tests
        Hide
        Jay Hill added a comment -

        Noble and Erik, thanks for the quick response. I just applied the patch and rebuilt. It doesn't matter what I enter as a param for commit, when the finish method executes requestParameters.commit always equals true:

        Using: curl 'http://localhost:8080/solr/indexer/books?command=full-import&commit=false'

        The commit is still occurring. I just woke up and tested, so I'll dig in a little more to try to find out what's up.

        Show
        Jay Hill added a comment - Noble and Erik, thanks for the quick response. I just applied the patch and rebuilt. It doesn't matter what I enter as a param for commit, when the finish method executes requestParameters.commit always equals true: Using: curl 'http://localhost:8080/solr/indexer/books?command=full-import&commit=false' The commit is still occurring. I just woke up and tested, so I'll dig in a little more to try to find out what's up.
        Hide
        Jay Hill added a comment -

        Looking at the handleRequestBody method of DataImportHandler, it looks like it is getting the correct value for "commit" from the request, but during the mapping to create the DataImporter.RequestParams object commit is always getting set as "true":

        SolrParams params = req.getParams();
        System.out.println(" -------------------From request : " + params.get("commit"));
        DataImporter.RequestParams requestParams = new DataImporter.RequestParams(getParamsMap(params));
        System.out.println(" -------------------RequestParams: " + requestParams.commit);

        the output:
        -------------------From request : false
        -------------------RequestParams: true

        still digging.

        Show
        Jay Hill added a comment - Looking at the handleRequestBody method of DataImportHandler, it looks like it is getting the correct value for "commit" from the request, but during the mapping to create the DataImporter.RequestParams object commit is always getting set as "true": SolrParams params = req.getParams(); System.out.println(" -------------------From request : " + params.get("commit")); DataImporter.RequestParams requestParams = new DataImporter.RequestParams(getParamsMap(params)); System.out.println(" -------------------RequestParams: " + requestParams.commit); the output: -------------------From request : false -------------------RequestParams: true still digging.
        Hide
        Jay Hill added a comment -

        Found the problem. There was a test to see if there was a value set for "optimize", if so, no matter what it was, "commit" was set to "true":
        if (requestParams.containsKey("optimize"))

        { optimize = Boolean.parseBoolean((String) requestParams.get("optimize")); commit = true; }

        But we had optimize=false set as an invariant so the simple presence of a value (false) caused a commit to happen. Changed it to this:
        if (requestParams.containsKey("optimize"))

        { optimize = Boolean.parseBoolean((String) requestParams.get("optimize")); if (optimize) commit = true; }

        I think that should do it.

        Show
        Jay Hill added a comment - Found the problem. There was a test to see if there was a value set for "optimize", if so, no matter what it was, "commit" was set to "true": if (requestParams.containsKey("optimize")) { optimize = Boolean.parseBoolean((String) requestParams.get("optimize")); commit = true; } But we had optimize=false set as an invariant so the simple presence of a value (false) caused a commit to happen. Changed it to this: if (requestParams.containsKey("optimize")) { optimize = Boolean.parseBoolean((String) requestParams.get("optimize")); if (optimize) commit = true; } I think that should do it.
        Hide
        Erik Hatcher added a comment -

        Committed as r796334. Thanks Noble and Jay!

        Show
        Erik Hatcher added a comment - Committed as r796334. Thanks Noble and Jay!
        Hide
        Grant Ingersoll added a comment -

        Bulk close for Solr 1.4

        Show
        Grant Ingersoll added a comment - Bulk close for Solr 1.4

          People

          • Assignee:
            Erik Hatcher
            Reporter:
            Jay Hill
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development