Solr
  1. Solr
  2. SOLR-5264

New method on NamedList to return one or many config arguments as collection

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.5
    • Fix Version/s: 4.6, Trunk
    • Component/s: clients - java
    • Labels:
      None

      Description

      In the FieldMutatingUpdateProcessorFactory is a method called "oneOrMany" that takes all of the entries in a NamedList and pulls them out into a Collection. I'd like to use that in a custom update processor I'm building.

      It seems as though this functionality would be right at home as part of NamedList itself. Here's a patch that moves the method.

      1. SOLR-5264-boolean.patch
        4 kB
        Shawn Heisey
      2. SOLR-5264.patch
        18 kB
        Shawn Heisey
      3. SOLR-5264.patch
        17 kB
        Shawn Heisey
      4. SOLR-5264.patch
        14 kB
        Shawn Heisey
      5. SOLR-5264.patch
        10 kB
        Shawn Heisey
      6. SOLR-5264.patch
        21 kB
        Shawn Heisey

        Activity

        Hide
        Shawn Heisey added a comment -

        Attaching patch. I'm sure there's a lot to not like about it, so I'd like comments in two areas. 1) Is the general idea sound? 2) What specifically could be done better?

        It did occur to me that the more generic method I mentioned in the TODO would in fact be the best approach right up front. That would name the method removeCollection instead of removeArgsCollection, returning Collection<Object>. It would then be up to the caller to decide what object types constitute an error condition. Thoughts?

        Show
        Shawn Heisey added a comment - Attaching patch. I'm sure there's a lot to not like about it, so I'd like comments in two areas. 1) Is the general idea sound? 2) What specifically could be done better? It did occur to me that the more generic method I mentioned in the TODO would in fact be the best approach right up front. That would name the method removeCollection instead of removeArgsCollection, returning Collection<Object>. It would then be up to the caller to decide what object types constitute an error condition. Thoughts?
        Hide
        Shawn Heisey added a comment -

        Patch isn't complete. I will fix and re-upload later.

        Show
        Shawn Heisey added a comment - Patch isn't complete. I will fix and re-upload later.
        Hide
        Shawn Heisey added a comment -

        Updated patch. No compilation errors. Running tests.

        Still need to make a test for the new method.

        Show
        Shawn Heisey added a comment - Updated patch. No compilation errors. Running tests. Still need to make a test for the new method.
        Hide
        Shawn Heisey added a comment -

        I've thought of a new approach. There is already a getAll method. I can implement a removeAll method that's very similar to getAll. I can then re-do the method I've created here, renaming it to removeArgs: Start with getAll. Go through that list and build a collection, throwing an exception if any found values are not Strings. Finally, delete the matching values and return the collection.

        Show
        Shawn Heisey added a comment - I've thought of a new approach. There is already a getAll method. I can implement a removeAll method that's very similar to getAll. I can then re-do the method I've created here, renaming it to removeArgs: Start with getAll. Go through that list and build a collection, throwing an exception if any found values are not Strings. Finally, delete the matching values and return the collection.
        Hide
        Shawn Heisey added a comment -

        New patch. Adds methods to NamedList: public methods removeAll and removeArgs, private method killAll. Removes the oneOrMany method from FieldMutatingUpdateProcessorFactory. Adds tests for new methods, improves one existing test. I am seeing a replication stress test failure, but I don't think it's related to these changes. The patch has a conflict on branch_4x, so I haven't done any testing there yet.

        All this so I can use removeArgs in my own custom update processors!

        Please let me know if there is any objection to committing this after making sure branch_4x is good.

        Show
        Shawn Heisey added a comment - New patch. Adds methods to NamedList: public methods removeAll and removeArgs, private method killAll. Removes the oneOrMany method from FieldMutatingUpdateProcessorFactory. Adds tests for new methods, improves one existing test. I am seeing a replication stress test failure, but I don't think it's related to these changes. The patch has a conflict on branch_4x, so I haven't done any testing there yet. All this so I can use removeArgs in my own custom update processors! Please let me know if there is any objection to committing this after making sure branch_4x is good.
        Hide
        Shawn Heisey added a comment -

        New patch. This one deprecates oneOrMany rather than removing it. Removal can be handled in a future version. I was thinking 4.7 or 4.8.

        Show
        Shawn Heisey added a comment - New patch. This one deprecates oneOrMany rather than removing it. Removal can be handled in a future version. I was thinking 4.7 or 4.8.
        Hide
        Steve Rowe added a comment -

        The idea seems good to me. All Solr tests pass for me on trunk with the patch applied.

        A few nits:

        • removeAll and removeArgs don't look very different, but they do different things. I think their javadocs should mention each other and say how they're different from each other.
        • I don't think removeArgs is a sufficiently descriptive name, but all attempts I've made to come up with a better one are way too long, like recursivelyRemoveAllStringArgs. But I think we could do better somehow here. oneOrMany is short but doesn't mention that the args are removed, so I don't think we should just use it.
        • The deprecation message on oneOrMany should say which version it will be removed in. I think 5.0 is a better choice than 4.7 or 4.8, since there is already established precedent for deprecation in major version X, removal in major version X+1. Then the trunk commit will remove it, and the branch_4x version will have the deprecation message, and there is nothing else to do (except a CHANGES.txt entry explaining this, which you should include when you commit), so this strategy is also easier to implement than removal in X.Y+1 or 2, since you have to remember to do that then.
        Show
        Steve Rowe added a comment - The idea seems good to me. All Solr tests pass for me on trunk with the patch applied. A few nits: removeAll and removeArgs don't look very different, but they do different things. I think their javadocs should mention each other and say how they're different from each other. I don't think removeArgs is a sufficiently descriptive name, but all attempts I've made to come up with a better one are way too long, like recursivelyRemoveAllStringArgs. But I think we could do better somehow here. oneOrMany is short but doesn't mention that the args are removed, so I don't think we should just use it. The deprecation message on oneOrMany should say which version it will be removed in. I think 5.0 is a better choice than 4.7 or 4.8, since there is already established precedent for deprecation in major version X, removal in major version X+1. Then the trunk commit will remove it, and the branch_4x version will have the deprecation message, and there is nothing else to do (except a CHANGES.txt entry explaining this, which you should include when you commit), so this strategy is also easier to implement than removal in X.Y+1 or 2, since you have to remember to do that then.
        Hide
        Shawn Heisey added a comment -

        Thanks for the feedback.

        • I like your idea of having removeAll and removeArgs mention each other in the javadoc. I'll do that.
        • I was really struggling trying to find a good name instead of removeArgs that wasn't horribly long, finally just decided to make it short. I'm open to suggestions in that department. The best alternative I've thought up so far is removeConfigArgs, which is more descriptive of its typical usage. I'm inclined to make that change unless there's strong objection.
        • Point taken on removal in 5.0 instead of a later 4.x release. That will be noted in the javadoc. Am I right in thinking that it's better to make an additional trunk commit to remove the method, rather than start in 4x and merge to trunk?
        Show
        Shawn Heisey added a comment - Thanks for the feedback. I like your idea of having removeAll and removeArgs mention each other in the javadoc. I'll do that. I was really struggling trying to find a good name instead of removeArgs that wasn't horribly long, finally just decided to make it short. I'm open to suggestions in that department. The best alternative I've thought up so far is removeConfigArgs, which is more descriptive of its typical usage. I'm inclined to make that change unless there's strong objection. Point taken on removal in 5.0 instead of a later 4.x release. That will be noted in the javadoc. Am I right in thinking that it's better to make an additional trunk commit to remove the method, rather than start in 4x and merge to trunk?
        Hide
        Shawn Heisey added a comment -

        New patch. Modifies javadoc as recommended. Renames removeArgs to removeConfigArgs. Does not seem to introduce any test failures.

        Show
        Shawn Heisey added a comment - New patch. Modifies javadoc as recommended. Renames removeArgs to removeConfigArgs. Does not seem to introduce any test failures.
        Hide
        Steve Rowe added a comment -

        +1 to commit.

        Am I right in thinking that it's better to make an additional trunk commit to remove the method, rather than start in 4x and merge to trunk?

        Mostly people start in trunk and backport to 4x, rather than the reverse, but I think either approach can make sense, depending on context. No hard and fast rule here.

        Show
        Steve Rowe added a comment - +1 to commit. Am I right in thinking that it's better to make an additional trunk commit to remove the method, rather than start in 4x and merge to trunk? Mostly people start in trunk and backport to 4x, rather than the reverse, but I think either approach can make sense, depending on context. No hard and fast rule here.
        Hide
        Shawn Heisey added a comment -

        Tests and precommit pass on trunk with patch.

        Show
        Shawn Heisey added a comment - Tests and precommit pass on trunk with patch.
        Hide
        ASF subversion and git services added a comment -

        Commit 1528680 from Shawn Heisey in branch 'dev/trunk'
        [ https://svn.apache.org/r1528680 ]

        SOLR-5264 - move config argument retrieval/removal from Update Processor code to NamedList

        Show
        ASF subversion and git services added a comment - Commit 1528680 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1528680 ] SOLR-5264 - move config argument retrieval/removal from Update Processor code to NamedList
        Hide
        Shawn Heisey added a comment -

        As I prepared to remove the deprecated method from trunk, I saw getBooleanArg. I really should give that method the same treatment.

        Show
        Shawn Heisey added a comment - As I prepared to remove the deprecated method from trunk, I saw getBooleanArg. I really should give that method the same treatment.
        Hide
        ASF subversion and git services added a comment -

        Commit 1528699 from Shawn Heisey in branch 'dev/trunk'
        [ https://svn.apache.org/r1528699 ]

        SOLR-5264 removing deprecated "oneOrMany" method from trunk

        Show
        ASF subversion and git services added a comment - Commit 1528699 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1528699 ] SOLR-5264 removing deprecated "oneOrMany" method from trunk
        Hide
        ASF subversion and git services added a comment -

        Commit 1528700 from Shawn Heisey in branch 'dev/trunk'
        [ https://svn.apache.org/r1528700 ]

        SOLR-5264 - CHANGES.txt. References boolean arg work that's not done yet.

        Show
        ASF subversion and git services added a comment - Commit 1528700 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1528700 ] SOLR-5264 - CHANGES.txt. References boolean arg work that's not done yet.
        Hide
        ASF subversion and git services added a comment -

        Commit 1528701 from Shawn Heisey in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1528701 ]

        SOLR-5264: merge trunk r1528680 and r1528700. move config argument retrieval/removal from Update Processor code to NamedList

        Show
        ASF subversion and git services added a comment - Commit 1528701 from Shawn Heisey in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1528701 ] SOLR-5264 : merge trunk r1528680 and r1528700. move config argument retrieval/removal from Update Processor code to NamedList
        Hide
        Shawn Heisey added a comment -

        Additional patch to move the method for removing/retrieving a boolean argument. Already mentioned in the CHANGES.txt that has been committed.

        Show
        Shawn Heisey added a comment - Additional patch to move the method for removing/retrieving a boolean argument. Already mentioned in the CHANGES.txt that has been committed.
        Hide
        Steve Rowe added a comment -

        Additional patch to move the method for removing/retrieving a boolean argument. Already mentioned in the CHANGES.txt that has been committed.

        +1

        Show
        Steve Rowe added a comment - Additional patch to move the method for removing/retrieving a boolean argument. Already mentioned in the CHANGES.txt that has been committed. +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1528976 from Shawn Heisey in branch 'dev/trunk'
        [ https://svn.apache.org/r1528976 ]

        SOLR-5264: move getBooleanArg to NamedList#removeBooleanArg

        Show
        ASF subversion and git services added a comment - Commit 1528976 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1528976 ] SOLR-5264 : move getBooleanArg to NamedList#removeBooleanArg
        Hide
        ASF subversion and git services added a comment -

        Commit 1529016 from Shawn Heisey in branch 'dev/trunk'
        [ https://svn.apache.org/r1529016 ]

        SOLR-5264: Removing deprecated getBooleanArg from trunk.

        Show
        ASF subversion and git services added a comment - Commit 1529016 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1529016 ] SOLR-5264 : Removing deprecated getBooleanArg from trunk.
        Hide
        ASF subversion and git services added a comment -

        Commit 1529020 from Shawn Heisey in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1529020 ]

        SOLR-5264: merge trunk r1528976, move getBooleanArg to NamedList#removeBooleanArg

        Show
        ASF subversion and git services added a comment - Commit 1529020 from Shawn Heisey in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1529020 ] SOLR-5264 : merge trunk r1528976, move getBooleanArg to NamedList#removeBooleanArg
        Hide
        Shawn Heisey added a comment -

        Before each individual commit, tests and precommit passed.

        Show
        Shawn Heisey added a comment - Before each individual commit, tests and precommit passed.

          People

          • Assignee:
            Shawn Heisey
            Reporter:
            Shawn Heisey
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development