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

SolrPluginUtils.invokeSetters should accommodate setter variants

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.2, 7.0
    • Component/s: None
    • Labels:
      None

      Description

      The code currently assumes that there is only one setter (or if there are several setters then the first one found is used and it could mismatch on the arg type).

      Context and motivation is that a class with a

      void setAFloat(float val) {
        this.val = val;
      }
      

      setter may wish to also provide a

      void setAFloat(String val) {
        this.val = Float.parseFloat(val);
      }
      

      convenience setter.

      1. SOLR-9161.patch
        4 kB
        Christine Poerschke
      2. SOLR-9161.patch
        4 kB
        Christine Poerschke

        Issue Links

          Activity

          Hide
          cpoerschke Christine Poerschke added a comment -

          Proposed patch including test.

          Show
          cpoerschke Christine Poerschke added a comment - Proposed patch including test.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 50658dd93d16eec37e906a24446146609cc93706 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=50658dd ]

          SOLR-9161: SolrPluginUtils.invokeSetters now accommodates setter variants

          Show
          jira-bot ASF subversion and git services added a comment - Commit 50658dd93d16eec37e906a24446146609cc93706 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=50658dd ] SOLR-9161 : SolrPluginUtils.invokeSetters now accommodates setter variants
          Hide
          steve_rowe Steve Rowe added a comment -

          My Jenkins found a seed that reproduces for me for SolrPluginUtils.testinvokeSetters():

          Checking out Revision 50658dd93d16eec37e906a24446146609cc93706 (refs/remotes/origin/master)
          [...]
             [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=SolrPluginUtilsTest -Dtests.method=testInvokeSetters -Dtests.seed=5040127289756128 -Dtests.slow=true -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt -Dtests.locale=ar-SY -Dtests.timezone=Asia/Kuching -Dtests.asserts=true -Dtests.file.encoding=UTF-8
             [junit4] ERROR   0.01s J6  | SolrPluginUtilsTest.testInvokeSetters <<<
             [junit4]    > Throwable #1: java.lang.IllegalArgumentException: argument type mismatch
             [junit4]    > 	at __randomizedtesting.SeedInfo.seed([5040127289756128:395FB9727C430383]:0)
             [junit4]    > 	at org.apache.solr.util.SolrPluginUtils.invokeSetters(SolrPluginUtils.java:1071)
             [junit4]    > 	at org.apache.solr.util.SolrPluginUtilsTest.implTestInvokeSetters(SolrPluginUtilsTest.java:482)
             [junit4]    > 	at org.apache.solr.util.SolrPluginUtilsTest.testInvokeSetters(SolrPluginUtilsTest.java:474)
             [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
          
          Show
          steve_rowe Steve Rowe added a comment - My Jenkins found a seed that reproduces for me for SolrPluginUtils.testinvokeSetters() : Checking out Revision 50658dd93d16eec37e906a24446146609cc93706 (refs/remotes/origin/master) [...] [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=SolrPluginUtilsTest -Dtests.method=testInvokeSetters -Dtests.seed=5040127289756128 -Dtests.slow=true -Dtests.linedocsfile=/home/jenkins/lucene-data/enwiki.random.lines.txt -Dtests.locale=ar-SY -Dtests.timezone=Asia/Kuching -Dtests.asserts=true -Dtests.file.encoding=UTF-8 [junit4] ERROR 0.01s J6 | SolrPluginUtilsTest.testInvokeSetters <<< [junit4] > Throwable #1: java.lang.IllegalArgumentException: argument type mismatch [junit4] > at __randomizedtesting.SeedInfo.seed([5040127289756128:395FB9727C430383]:0) [junit4] > at org.apache.solr.util.SolrPluginUtils.invokeSetters(SolrPluginUtils.java:1071) [junit4] > at org.apache.solr.util.SolrPluginUtilsTest.implTestInvokeSetters(SolrPluginUtilsTest.java:482) [junit4] > at org.apache.solr.util.SolrPluginUtilsTest.testInvokeSetters(SolrPluginUtilsTest.java:474) [junit4] > at java.lang.Thread.run(Thread.java:745)
          Hide
          thetaphi Uwe Schindler added a comment -

          Happens for me, too, on every JVM version.

          FYI, I implemented something similar for the extraction module or also for the SystemInfoHandler: https://github.com/apache/lucene-solr/blob/5e5fd662575105de88d8514b426bccdcb4c76948/solr/core/src/java/org/apache/solr/handler/admin/SystemInfoHandler.java#L214-L232

          It uses the java.beans.Introspector class to introspect beans and read/set properties. This should be preferred, if you need to guess method names (setProperty, getProperty and isProperty for booleans).

          I think the same should be done for the plugin utils, if they use bean properties.

          Show
          thetaphi Uwe Schindler added a comment - Happens for me, too, on every JVM version. FYI, I implemented something similar for the extraction module or also for the SystemInfoHandler: https://github.com/apache/lucene-solr/blob/5e5fd662575105de88d8514b426bccdcb4c76948/solr/core/src/java/org/apache/solr/handler/admin/SystemInfoHandler.java#L214-L232 It uses the java.beans.Introspector class to introspect beans and read/set properties. This should be preferred, if you need to guess method names (setProperty, getProperty and isProperty for booleans). I think the same should be done for the plugin utils, if they use bean properties.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 245e4839d1e50ed05a59a2f2ee82be713cc7e6fc in lucene-solr's branch refs/heads/master from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=245e483 ]

          Revert "SOLR-9161: SolrPluginUtils.invokeSetters now accommodates setter variants"

          This reverts commit 50658dd93d16eec37e906a24446146609cc93706.

          Conflicts:
          solr/CHANGES.txt

          Show
          jira-bot ASF subversion and git services added a comment - Commit 245e4839d1e50ed05a59a2f2ee82be713cc7e6fc in lucene-solr's branch refs/heads/master from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=245e483 ] Revert " SOLR-9161 : SolrPluginUtils.invokeSetters now accommodates setter variants" This reverts commit 50658dd93d16eec37e906a24446146609cc93706. Conflicts: solr/CHANGES.txt
          Hide
          hossman Hoss Man added a comment -

          Since these test failures were so reliablly reproducing, and happening on so many seeds, and this change didn't seem to be a dependency for any other existing functionality on master, i went ahead and reverted until Christine can have a chance to dig into the failures more.

          Show
          hossman Hoss Man added a comment - Since these test failures were so reliablly reproducing, and happening on so many seeds, and this change didn't seem to be a dependency for any other existing functionality on master, i went ahead and reverted until Christine can have a chance to dig into the failures more.
          Hide
          cpoerschke Christine Poerschke added a comment -

          Thanks Steve, Uwe and Hoss. Apologies, I haven't had time yet to look into this further.

          Show
          cpoerschke Christine Poerschke added a comment - Thanks Steve, Uwe and Hoss. Apologies, I haven't had time yet to look into this further.
          Hide
          cpoerschke Christine Poerschke added a comment -

          Unable to reproduce the test failure locally (with that seed).

          Attaching alternative patch using the java.beans.Introspector class as Uwe suggested, the tests pass locally for me (but then they also did so with the original patch).

          Steve, Uwe - when you have a spare moment, would you mind applying/running the test locally to you? If all is well I'd be aiming to commit the change sometime next week.

          Show
          cpoerschke Christine Poerschke added a comment - Unable to reproduce the test failure locally (with that seed). Attaching alternative patch using the java.beans.Introspector class as Uwe suggested, the tests pass locally for me (but then they also did so with the original patch). Steve, Uwe - when you have a spare moment, would you mind applying/running the test locally to you? If all is well I'd be aiming to commit the change sometime next week.
          Hide
          steve_rowe Steve Rowe added a comment -

          Christine, I went back and, using Miller's beasting script, beasted SolrPluginUtilsTest for 100 iterations on the revision just before Hoss reverted, and it failed 72/100 iterations. My seed above also failed for me at that revision.

          I then applied your latest patch to current master and again beasted 100 iterations of SolrPluginUtilsTest, and it failed 0/100 iterations. (My reported seed also did not fail, but I'm not sure whether that's useful information.)

          Show
          steve_rowe Steve Rowe added a comment - Christine, I went back and, using Miller's beasting script, beasted SolrPluginUtilsTest for 100 iterations on the revision just before Hoss reverted, and it failed 72/100 iterations. My seed above also failed for me at that revision. I then applied your latest patch to current master and again beasted 100 iterations of SolrPluginUtilsTest , and it failed 0/100 iterations. (My reported seed also did not fail, but I'm not sure whether that's useful information.)
          Hide
          cpoerschke Christine Poerschke added a comment -

          Thanks Steve. I also just ran the beasting script and it failed 0/100 iterations.

          cd solr/core
          ant beast -Dbeast.iters=100 -Dtestcase=SolrPluginUtilsTest
          
          Show
          cpoerschke Christine Poerschke added a comment - Thanks Steve. I also just ran the beasting script and it failed 0/100 iterations. cd solr/core ant beast -Dbeast.iters=100 -Dtestcase=SolrPluginUtilsTest
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 038fe9378dab18d0e16b34c26dc802c6560e77e7 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=038fe93 ]

          SOLR-9161: change SolrPluginUtils.invokeSetters implementation to accommodate setter variants

          Show
          jira-bot ASF subversion and git services added a comment - Commit 038fe9378dab18d0e16b34c26dc802c6560e77e7 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=038fe93 ] SOLR-9161 : change SolrPluginUtils.invokeSetters implementation to accommodate setter variants
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9be5b98eb3ca85b7597f96dc9a42551fe3051d4d in lucene-solr's branch refs/heads/branch_6x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9be5b98 ]

          SOLR-9161: change SolrPluginUtils.invokeSetters implementation to accommodate setter variants

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9be5b98eb3ca85b7597f96dc9a42551fe3051d4d in lucene-solr's branch refs/heads/branch_6x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9be5b98 ] SOLR-9161 : change SolrPluginUtils.invokeSetters implementation to accommodate setter variants
          Hide
          mikemccand Michael McCandless added a comment -

          Bulk close resolved issues after 6.2.0 release.

          Show
          mikemccand Michael McCandless added a comment - Bulk close resolved issues after 6.2.0 release.

            People

            • Assignee:
              cpoerschke Christine Poerschke
              Reporter:
              cpoerschke Christine Poerschke
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development