Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-3883

distributed indexing forwards non-applicable request params

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0, 6.0
    • Component/s: SolrCloud
    • Labels:
      None

      Description

      Including all of the original request params can cause really undesirable stuff to happen.

      Reported by Dan Sutton:
      http://find.searchhub.org/document/c21c27f20e115ce9

        Activity

        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Per had brought this up as a possible issue in the past, but it was tied up in an issue with a lot of other stuff.

        Was trying to avoid having to guess everything that should go, but looks like the reverse may be worse.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Per had brought this up as a possible issue in the past, but it was tied up in an issue with a lot of other stuff. Was trying to avoid having to guess everything that should go, but looks like the reverse may be worse.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        First guess is that it's probably safer/easier to add params we know we need rather than remove params that could do things we don't want? It just seems like that as we add new methods of creating documents in the future, "dangerous" params could proliferate. Seems like it may be easier to enumerate those params that are necessary/desirable for updates.

        Brainstorming off the top of my head:

        • Params for updates: commitWithin, NOW?
        • Params for commits: softCommit, waitSearcher?
        Show
        yseeley@gmail.com Yonik Seeley added a comment - First guess is that it's probably safer/easier to add params we know we need rather than remove params that could do things we don't want? It just seems like that as we add new methods of creating documents in the future, "dangerous" params could proliferate. Seems like it may be easier to enumerate those params that are necessary/desirable for updates. Brainstorming off the top of my head: Params for updates: commitWithin, NOW? Params for commits: softCommit, waitSearcher?
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Params for updates: commitWithin, NOW? Params for commits: softCommit, waitSearcher?

        I've got to brush up on the NOW param...

        I think the commit params are all fine - we build a command from the orig request and then pass that on.

        We do need to do UpdateParams.UPDATE_CHAIN, but I am not 100% on what else yet.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Params for updates: commitWithin, NOW? Params for commits: softCommit, waitSearcher? I've got to brush up on the NOW param... I think the commit params are all fine - we build a command from the orig request and then pass that on. We do need to do UpdateParams.UPDATE_CHAIN, but I am not 100% on what else yet.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Here is an initial patch that only pass UPDATE_CHAIN - no handling of NOW.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Here is an initial patch that only pass UPDATE_CHAIN - no handling of NOW.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Yeah, I guess commit params are already handled within the command - no need for request params.
        Is this also the case for commitWithin on adds, deletes, deleteByQuery?

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Yeah, I guess commit params are already handled within the command - no need for request params. Is this also the case for commitWithin on adds, deletes, deleteByQuery?
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Is this also the case for commitWithin on adds, deletes, deleteByQuery?

        I suppose not - this just covers the commit case. It also covers commitWithin from xml or whatever, but it seems we do also have to worry about the case where it's just a request params on an add or delete.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Is this also the case for commitWithin on adds, deletes, deleteByQuery? I suppose not - this just covers the commit case. It also covers commitWithin from xml or whatever, but it seems we do also have to worry about the case where it's just a request params on an add or delete.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        I suppose not

        I take that back - looking further, it seems all ways commitWithin can come in, it gets set on the update command, and our update command distributor then respects sending them on.

        Show
        markrmiller@gmail.com Mark Miller added a comment - I suppose not I take that back - looking further, it seems all ways commitWithin can come in, it gets set on the update command, and our update command distributor then respects sending them on.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        I think NOW could probably be considered it's own separate issue. I think we should get this change in. Anything I still may be missing?

        Show
        markrmiller@gmail.com Mark Miller added a comment - I think NOW could probably be considered it's own separate issue. I think we should get this change in. Anything I still may be missing?
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        I've committed the work so far.

        Show
        markrmiller@gmail.com Mark Miller added a comment - I've committed the work so far.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Okay, barring further feedback from yonik, I'm calling this resolved. We can open a new issue to consider how we want to handle NOW values with distrib indexing.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Okay, barring further feedback from yonik, I'm calling this resolved. We can open a new issue to consider how we want to handle NOW values with distrib indexing.
        Hide
        commit-tag-bot Commit Tag Bot added a comment -

        [branch_4x commit] Mark Robert Miller
        http://svn.apache.org/viewvc?view=revision&revision=1390274

        SOLR-3883: Distributed indexing forwards non-applicable request params.

        Show
        commit-tag-bot Commit Tag Bot added a comment - [branch_4x commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1390274 SOLR-3883 : Distributed indexing forwards non-applicable request params.
        Hide
        commit-tag-bot Commit Tag Bot added a comment -

        [branch_4x commit] Mark Robert Miller
        http://svn.apache.org/viewvc?view=revision&revision=1390274

        SOLR-3883: Distributed indexing forwards non-applicable request params.

        Show
        commit-tag-bot Commit Tag Bot added a comment - [branch_4x commit] Mark Robert Miller http://svn.apache.org/viewvc?view=revision&revision=1390274 SOLR-3883 : Distributed indexing forwards non-applicable request params.
        Hide
        thetaphi Uwe Schindler added a comment -

        Closed after release.

        Show
        thetaphi Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            markrmiller@gmail.com Mark Miller
            Reporter:
            yseeley@gmail.com Yonik Seeley
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development