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

Deprecate CSVStrategy's setters, and make its pre-configured strategies immutable

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.0
    • Component/s: None
    • Labels:
      None

      Description

      Removing some deprecated things in CSVStrategy (SOLR-8764) exposed a bug: it's possible to redefine the public static CSVStrategy.{DEFAULT,EXCEL,TDF}_STRATEGY strategies, simply by calling their setters.

      Right now that's happening in CSVParserTest.testUnicodeEscape(), where the default unicode escape interpretation is changed from false to true. And then if that test happens to run before CSVStrategyTest.testSetCSVStrategy(), which tests that the unicode escape interpretation on the default strategy is set to false, then the latter will fail.

      Example failures: http://jenkins.thetaphi.de/job/Lucene-Solr-trunk-Linux/16079/ and http://jenkins.thetaphi.de/job/Lucene-Solr-trunk-MacOSX/3126/

        Issue Links

          Activity

          Hide
          steve_rowe Steve Rowe added a comment -

          I don't think it's useful to have live setters at all on a CSVStrategy instance - they should be removed.

          I'll make a patch that:

          • deprecates the setters (to be removed in 7.0, sigh)
          • switches the ctors to not call the setters
          • switches all internal Solr usages to treat CSVStrategy instances as if they were immutable (i.e., don't call any setters)
          • adds a deprecated private CSVStrategy subclass that throws an error from all setters, to be used for the pre-defined static strategies
          Show
          steve_rowe Steve Rowe added a comment - I don't think it's useful to have live setters at all on a CSVStrategy instance - they should be removed. I'll make a patch that: deprecates the setters (to be removed in 7.0, sigh) switches the ctors to not call the setters switches all internal Solr usages to treat CSVStrategy instances as if they were immutable (i.e., don't call any setters) adds a deprecated private CSVStrategy subclass that throws an error from all setters, to be used for the pre-defined static strategies
          Hide
          steve_rowe Steve Rowe added a comment -

          Patch implementing the idea, additionally adding a ctor param for printerNewline and deprecating clone(), which will also be removed (along with implements Cloneable) in Solr 7, since the sequence (clone(),setXXX(), ...) will no longer be supported - people will just create new immutable instances using one of the ctors.

          I guess a builder would make sense here, since there are now 9 params in the full ctor, but I don't plan on doing that.

          I'll make a followup issue to remove the deprecated stuff in master.

          Precommit passes, along with the unit tests in the same package. Running the full Solr test suite now, once done, assuming no failures, I'll commit.

          Show
          steve_rowe Steve Rowe added a comment - Patch implementing the idea, additionally adding a ctor param for printerNewline and deprecating clone() , which will also be removed (along with implements Cloneable ) in Solr 7, since the sequence (clone(),setXXX(), ...) will no longer be supported - people will just create new immutable instances using one of the ctors. I guess a builder would make sense here, since there are now 9 params in the full ctor, but I don't plan on doing that. I'll make a followup issue to remove the deprecated stuff in master. Precommit passes, along with the unit tests in the same package. Running the full Solr test suite now, once done, assuming no failures, I'll commit.
          Hide
          thetaphi Uwe Schindler added a comment -

          +1 to fix this!

          Ideally we should remove our CSV fork from Solr's repository! I tried to do that and cut over to the official release, but I gave up; see SOLR-3213. The commons-csv release is a total rewrite not having any overlap with our early fork in the internal package. The official release does not support most "hackish" stuff, introduced to spare allocations (which is no longer an issue in Java 8, we can do new CSV*() as often as we like, Hotspot optimizes that away). I think this stuff also is the reason for this issue, Steve Rowe?

          So we should really upgrade to a better CSV parser (official commons-csv) and remove the "reuse" of instances in thread unsafe ways.

          Show
          thetaphi Uwe Schindler added a comment - +1 to fix this! Ideally we should remove our CSV fork from Solr's repository! I tried to do that and cut over to the official release, but I gave up; see SOLR-3213 . The commons-csv release is a total rewrite not having any overlap with our early fork in the internal package. The official release does not support most "hackish" stuff, introduced to spare allocations (which is no longer an issue in Java 8, we can do new CSV*() as often as we like, Hotspot optimizes that away). I think this stuff also is the reason for this issue, Steve Rowe ? So we should really upgrade to a better CSV parser (official commons-csv) and remove the "reuse" of instances in thread unsafe ways.
          Hide
          steve_rowe Steve Rowe added a comment -

          +1, after the 6.0 release. I'll commit the patch for now.

          Show
          steve_rowe Steve Rowe added a comment - +1, after the 6.0 release. I'll commit the patch for now.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit a079ff2528a384468289a8fb80787b418c887496 in lucene-solr's branch refs/heads/master from Steve Rowe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a079ff2 ]

          SOLR-8778: Deprecate CSVStrategy's setters, and make its pre-configured strategies immutable

          Show
          jira-bot ASF subversion and git services added a comment - Commit a079ff2528a384468289a8fb80787b418c887496 in lucene-solr's branch refs/heads/master from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a079ff2 ] SOLR-8778 : Deprecate CSVStrategy's setters, and make its pre-configured strategies immutable
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit eb0c3bd90a4feee61b7c9b4c21a78a9f9bace0ff in lucene-solr's branch refs/heads/branch_6x from Steve Rowe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eb0c3bd ]

          SOLR-8778: Deprecate CSVStrategy's setters, and make its pre-configured strategies immutable

          Show
          jira-bot ASF subversion and git services added a comment - Commit eb0c3bd90a4feee61b7c9b4c21a78a9f9bace0ff in lucene-solr's branch refs/heads/branch_6x from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eb0c3bd ] SOLR-8778 : Deprecate CSVStrategy's setters, and make its pre-configured strategies immutable
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 8034b5aabd17aa60a37417359841623b07a9e927 in lucene-solr's branch refs/heads/branch_6_0 from Steve Rowe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8034b5a ]

          SOLR-8778: Deprecate CSVStrategy's setters, and make its pre-configured strategies immutable

          Show
          jira-bot ASF subversion and git services added a comment - Commit 8034b5aabd17aa60a37417359841623b07a9e927 in lucene-solr's branch refs/heads/branch_6_0 from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8034b5a ] SOLR-8778 : Deprecate CSVStrategy's setters, and make its pre-configured strategies immutable
          Hide
          steve_rowe Steve Rowe added a comment -

          Resolving as fixed. I'll hold off creating a JIRA to remove the deprecated stuff in master, since as Uwe says we should instead remove the whole fork.

          Show
          steve_rowe Steve Rowe added a comment - Resolving as fixed. I'll hold off creating a JIRA to remove the deprecated stuff in master, since as Uwe says we should instead remove the whole fork.
          Hide
          thetaphi Uwe Schindler added a comment -

          I'll hold off creating a JIRA to remove the deprecated stuff in master, since as Uwe says we should instead remove the whole fork.

          We should investigate this. Issue with patch is SOLR-3213, but I gave up because to fix this you have to understand the whole CSV handler code (and I was not familar to it when I tried to do this).

          Show
          thetaphi Uwe Schindler added a comment - I'll hold off creating a JIRA to remove the deprecated stuff in master, since as Uwe says we should instead remove the whole fork. We should investigate this. Issue with patch is SOLR-3213 , but I gave up because to fix this you have to understand the whole CSV handler code (and I was not familar to it when I tried to do this).

            People

            • Assignee:
              steve_rowe Steve Rowe
              Reporter:
              steve_rowe Steve Rowe
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development