Solr
  1. Solr
  2. SOLR-1647

Remove the option of setting solrconfig from web.xml

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0
    • Component/s: None
    • Labels:
      None

      Description

      with SOLR-1621 , it is not required to have an option to set solrconfig from web.xml. Moreover editing web.xml means hacking solr itself.

        Activity

        Noble Paul created issue -
        Noble Paul made changes -
        Field Original Value New Value
        Parent SOLR-1621 [ 12442418 ]
        Issue Type Sub-task [ 7 ] Improvement [ 4 ]
        Hide
        Noble Paul added a comment -

        this can be taken up as a separate issue . SOLR-1621 can be committed w/o this

        Show
        Noble Paul added a comment - this can be taken up as a separate issue . SOLR-1621 can be committed w/o this
        Noble Paul made changes -
        Attachment SOLR-1647.patch [ 12428162 ]
        Hide
        Noble Paul added a comment -

        I plan to commit this shortly.

        Show
        Noble Paul added a comment - I plan to commit this shortly.
        Hide
        Mark Miller added a comment -

        -1 on this patch.

        1. There is no reason not to keep this for a release with a warning to users that its going to be removed. You put in a deprecation tag in an earlier patch, and now your just removing it before that was ever released - there is no downside to doing this more like a real deprecation and there is upside.

        2. You have put no warnings or information to users about this change - you could be breaking things for some people and your doing it out of the blue - we shouldn't pull the rug out from under users like that.

        3. Ive I have said more than once, the current patch breaks tests that rely on this feature.

        I see no reason to remove it now. It should be deprecated with a warning in changes about its impending removal. And you need to provide a workaround for tests. I don't see why we don't just keep it for now. Its not hurting anything.

        I'd also like to be more clear on what deprecation means in Solr - I know it doesn't mean deprecating and removing before a release even goes out (that makes no sense) - and I'm not sure if it means removing in the next dot release (as Ive see that occurring recently). Can someone pipe in on whether Solr has an (in)formal policy on when deprecations should/can be removed?

        Show
        Mark Miller added a comment - -1 on this patch. 1. There is no reason not to keep this for a release with a warning to users that its going to be removed. You put in a deprecation tag in an earlier patch, and now your just removing it before that was ever released - there is no downside to doing this more like a real deprecation and there is upside. 2. You have put no warnings or information to users about this change - you could be breaking things for some people and your doing it out of the blue - we shouldn't pull the rug out from under users like that. 3. Ive I have said more than once, the current patch breaks tests that rely on this feature. I see no reason to remove it now. It should be deprecated with a warning in changes about its impending removal. And you need to provide a workaround for tests. I don't see why we don't just keep it for now. Its not hurting anything. I'd also like to be more clear on what deprecation means in Solr - I know it doesn't mean deprecating and removing before a release even goes out (that makes no sense) - and I'm not sure if it means removing in the next dot release (as Ive see that occurring recently). Can someone pipe in on whether Solr has an (in)formal policy on when deprecations should/can be removed?
        Hide
        Noble Paul added a comment -

        I wish to remove this feature because it is a bad one not because it is hard to keep it. I am sure there are very few users using it anyway.

        We can deprecate it and remove it later. But the question is do we have to be that strict about everything?

        Show
        Noble Paul added a comment - I wish to remove this feature because it is a bad one not because it is hard to keep it. I am sure there are very few users using it anyway. We can deprecate it and remove it later. But the question is do we have to be that strict about everything?
        Hide
        Mark Miller added a comment -

        But the question is do we have to be that strict about everything?

        Personally, I would say no, we don't (and many times we have not been). I'm not -1'ing the removal of this. I think we should keep it and say its going away first, but if we didn't, I wouldn't be too upset about it.

        The weight of my opposition comes from: you said your about to commit it but there is no evidence of what your going to do about warning users - you are removing a feature some may currently be counting on - and there is no work around for the tests you are breaking that rely on this feature. One fails, and others that count on it that don't fail now, will confuse people when they try and change the config for the tests, but the specified config is not actually being used.

        Show
        Mark Miller added a comment - But the question is do we have to be that strict about everything? Personally, I would say no, we don't (and many times we have not been). I'm not -1'ing the removal of this. I think we should keep it and say its going away first, but if we didn't, I wouldn't be too upset about it. The weight of my opposition comes from: you said your about to commit it but there is no evidence of what your going to do about warning users - you are removing a feature some may currently be counting on - and there is no work around for the tests you are breaking that rely on this feature. One fails, and others that count on it that don't fail now, will confuse people when they try and change the config for the tests, but the specified config is not actually being used.
        Hide
        Noble Paul added a comment -

        I'm definitely not going to commit the same patch which is attached. I will ensure that all tests pass before this goes in.

        I was trying to find what do others have to say on this.

        The warning is going to go in CHANGES.TXT. Do we have to put it anywhere else?

        Show
        Noble Paul added a comment - I'm definitely not going to commit the same patch which is attached. I will ensure that all tests pass before this goes in. I was trying to find what do others have to say on this. The warning is going to go in CHANGES.TXT. Do we have to put it anywhere else?
        Hide
        Yonik Seeley added a comment -

        I'd also like to be more clear on what deprecation means in Solr - I know it doesn't mean deprecating and removing before a release even goes out (that makes no sense) - and I'm not sure if it means removing in the next dot release (as Ive see that occurring recently). Can someone pipe in on whether Solr has an (in)formal policy on when deprecations should/can be removed?

        We don't really have a policy, but it's been a low priority to remove deprecations if they don't get in the way. I think it would depend a lot on what was deprecated (how expert-level or not).

        Specific to this feature, if it isn't causing problems with development of other features, there's no reason to not deprecate it first.

        Show
        Yonik Seeley added a comment - I'd also like to be more clear on what deprecation means in Solr - I know it doesn't mean deprecating and removing before a release even goes out (that makes no sense) - and I'm not sure if it means removing in the next dot release (as Ive see that occurring recently). Can someone pipe in on whether Solr has an (in)formal policy on when deprecations should/can be removed? We don't really have a policy, but it's been a low priority to remove deprecations if they don't get in the way. I think it would depend a lot on what was deprecated (how expert-level or not). Specific to this feature, if it isn't causing problems with development of other features, there's no reason to not deprecate it first.
        Hide
        Mark Miller added a comment - - edited

        I'm definitely not going to commit the same patch which is attached. I will ensure that all tests pass before this goes in.

        I guess this is a communication error. I took

        I plan to commit this shortly.

        as "you are going to commit the current patch".

        I guessed that you might do a few things before committing, but I have no way of knowing. When someone says that they plan to commit something shortly,
        I take it to mean something along the lines of the patch posted. As you are missing two things that are pretty major pieces to this patch (the deprecation/non deprecation approach and a good workaround
        for the tests), I voiced my opposition to the current approach shown. Its hard for me to guess what changes you will make to this patch before you commit soon - I have to assume when you say that you are committing shortly that perhaps you will address both things correctly and perhaps you won't - you could just commit the current patch, who knows - I can't really rely on you doing anything unless you post the patch first, but you mention nothing of another patch, just of committing. Thats why I brought up the issues that I did. I can see making some last minutes changes to a patch, but these two things are fairly important to this issue I feel, and not really last minute tweaks before a commit.

        I'd like the opportunity to take a look at how you are going to address these two issues and (fwiw) possibly provide feedback.

        Show
        Mark Miller added a comment - - edited I'm definitely not going to commit the same patch which is attached. I will ensure that all tests pass before this goes in. I guess this is a communication error. I took I plan to commit this shortly. as "you are going to commit the current patch". I guessed that you might do a few things before committing, but I have no way of knowing. When someone says that they plan to commit something shortly, I take it to mean something along the lines of the patch posted. As you are missing two things that are pretty major pieces to this patch (the deprecation/non deprecation approach and a good workaround for the tests), I voiced my opposition to the current approach shown. Its hard for me to guess what changes you will make to this patch before you commit soon - I have to assume when you say that you are committing shortly that perhaps you will address both things correctly and perhaps you won't - you could just commit the current patch, who knows - I can't really rely on you doing anything unless you post the patch first, but you mention nothing of another patch, just of committing. Thats why I brought up the issues that I did. I can see making some last minutes changes to a patch, but these two things are fairly important to this issue I feel, and not really last minute tweaks before a commit. I'd like the opportunity to take a look at how you are going to address these two issues and (fwiw) possibly provide feedback.
        Hide
        Hoss Man added a comment -

        Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email...

        http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

        Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed.

        A unique token for finding these 240 issues in the future: hossversioncleanup20100527

        Show
        Hoss Man added a comment - Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed. A unique token for finding these 240 issues in the future: hossversioncleanup20100527
        Hoss Man made changes -
        Fix Version/s Next [ 12315093 ]
        Fix Version/s 1.5 [ 12313566 ]
        Hoss Man made changes -
        Fix Version/s 3.2 [ 12316172 ]
        Fix Version/s Next [ 12315093 ]
        Hide
        Robert Muir added a comment -

        Bulk move 3.2 -> 3.3

        Show
        Robert Muir added a comment - Bulk move 3.2 -> 3.3
        Robert Muir made changes -
        Fix Version/s 3.3 [ 12316471 ]
        Fix Version/s 3.2 [ 12316172 ]
        Robert Muir made changes -
        Fix Version/s 3.4 [ 12316683 ]
        Fix Version/s 4.0 [ 12314992 ]
        Fix Version/s 3.3 [ 12316471 ]
        Hide
        Robert Muir added a comment -

        3.4 -> 3.5

        Show
        Robert Muir added a comment - 3.4 -> 3.5
        Robert Muir made changes -
        Fix Version/s 3.5 [ 12317876 ]
        Fix Version/s 3.4 [ 12316683 ]
        Simon Willnauer made changes -
        Fix Version/s 3.6 [ 12319065 ]
        Fix Version/s 3.5 [ 12317876 ]
        Robert Muir made changes -
        Fix Version/s 3.6 [ 12319065 ]
        Hide
        Hoss Man added a comment -

        bulk fixing the version info for 4.0-ALPHA and 4.0 all affected issues have "hoss20120711-bulk-40-change" in comment

        Show
        Hoss Man added a comment - bulk fixing the version info for 4.0-ALPHA and 4.0 all affected issues have "hoss20120711-bulk-40-change" in comment
        Hoss Man made changes -
        Fix Version/s 4.0 [ 12322455 ]
        Fix Version/s 4.0-ALPHA [ 12314992 ]
        Hide
        Robert Muir added a comment -

        rmuir20120906-bulk-40-change

        Show
        Robert Muir added a comment - rmuir20120906-bulk-40-change
        Robert Muir made changes -
        Fix Version/s 4.0 [ 12322551 ]
        Fix Version/s 4.0-BETA [ 12322455 ]
        Hide
        Hoss Man added a comment -

        CoreContainer.setSolrConfigFilename seems to have been removed at some point in the past, resolving.

        Show
        Hoss Man added a comment - CoreContainer.setSolrConfigFilename seems to have been removed at some point in the past, resolving.
        Hoss Man made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.
        Uwe Schindler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Noble Paul
            Reporter:
            Noble Paul
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development