Details

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

      Description

      CorePropertiesLocator currently does all its file system interaction using java.io.File and friends, which have all sorts of drawbacks with regard to error handling and reporting. We've been on java 7 for a while now, so we should use the nio2 Path APIs instead.

      1. SOLR-8260.patch
        18 kB
        Alan Woodward

        Issue Links

          Activity

          Hide
          Alan Woodward added a comment -

          Patch. Tests pass, running precommit now.

          Show
          Alan Woodward added a comment - Patch. Tests pass, running precommit now.
          Hide
          ASF subversion and git services added a comment -

          Commit 1713490 from Alan Woodward in branch 'dev/trunk'
          [ https://svn.apache.org/r1713490 ]

          SOLR-8260: Use nio2 API in core discovery

          Show
          ASF subversion and git services added a comment - Commit 1713490 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1713490 ] SOLR-8260 : Use nio2 API in core discovery
          Hide
          ASF subversion and git services added a comment -

          Commit 1713498 from Alan Woodward in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1713498 ]

          SOLR-8260: Use nio2 API in core discovery

          Show
          ASF subversion and git services added a comment - Commit 1713498 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1713498 ] SOLR-8260 : Use nio2 API in core discovery
          Hide
          Uwe Schindler added a comment -

          Thanks Alan! I like this very much. We should on working to disallow java.io.File throughout Solr!

          Show
          Uwe Schindler added a comment - Thanks Alan! I like this very much. We should on working to disallow java.io.File throughout Solr!
          Hide
          Alan Woodward added a comment -

          We should on working to disallow java.io.File throughout Solr!

          That would be great, but I think it might take a while!

          Show
          Alan Woodward added a comment - We should on working to disallow java.io.File throughout Solr! That would be great, but I think it might take a while!
          Hide
          Shawn Heisey added a comment -

          We should on working to disallow java.io.File throughout Solr!

          I thought this had already been added to the forbidden apis config in the build system, but it seems that was only for Lucene. I'm guessing that if we move it from the lucene config to the base config, Solr will pick it up as well.

          Show
          Shawn Heisey added a comment - We should on working to disallow java.io.File throughout Solr! I thought this had already been added to the forbidden apis config in the build system, but it seems that was only for Lucene. I'm guessing that if we move it from the lucene config to the base config, Solr will pick it up as well.
          Hide
          ASF subversion and git services added a comment -

          Commit 1713563 from Alan Woodward in branch 'dev/trunk'
          [ https://svn.apache.org/r1713563 ]

          SOLR-8260: Remove morphlines test setup with wrongly configured core discovery roots

          Show
          ASF subversion and git services added a comment - Commit 1713563 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1713563 ] SOLR-8260 : Remove morphlines test setup with wrongly configured core discovery roots
          Hide
          ASF subversion and git services added a comment -

          Commit 1713564 from Alan Woodward in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1713564 ]

          SOLR-8260: Remove morphlines test setup with wrongly configured core discovery roots

          Show
          ASF subversion and git services added a comment - Commit 1713564 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1713564 ] SOLR-8260 : Remove morphlines test setup with wrongly configured core discovery roots
          Hide
          Kevin Risden added a comment -

          Alan Woodward I see that you are going through and fixing a bunch to nio from java.io. Is there a JIRA outlining what still needs to be done?

          Show
          Kevin Risden added a comment - Alan Woodward I see that you are going through and fixing a bunch to nio from java.io. Is there a JIRA outlining what still needs to be done?
          Hide
          Alan Woodward added a comment -

          There isn't, but an overarching JIRA ticket isn't a bad idea. Will open one up!

          Show
          Alan Woodward added a comment - There isn't, but an overarching JIRA ticket isn't a bad idea. Will open one up!
          Hide
          Shawn Heisey added a comment -

          I added the same exclusions found in the forbiddenapi config for Lucene to Solr, and started going through the tagged classes in trunk to change to NIO2. I don't think I've touched anything for core discovery yet, so I'm hoping the commits here won't overlap.

          Show
          Shawn Heisey added a comment - I added the same exclusions found in the forbiddenapi config for Lucene to Solr, and started going through the tagged classes in trunk to change to NIO2. I don't think I've touched anything for core discovery yet, so I'm hoping the commits here won't overlap.
          Hide
          Kevin Risden added a comment -

          Probably makes sense to look over at SOLR-8282 that Alan Woodward created.

          Show
          Kevin Risden added a comment - Probably makes sense to look over at SOLR-8282 that Alan Woodward created.
          Hide
          David Smiley added a comment -

          I just reviewed this patch. One thing I'm seeing here that I usually consider to be a (minor) error is this pattern:

              catch (IOException e) {
                logger.error("Couldn't persist core properties to {}: {}", propfile, e.getMessage());
              }
          

          Two things:

          • I almost always pass the exception (variable e here) to the logger. Or in other cases involving throwing different exceptions, the parallel is forgetting to pass the exception as the cause. Was not doing this intentional?
          • To put the details of the exception in the logged string, I suggest e.toString() as it includes the class as well as the message. Forgetting to do this can be confusing, like for example if you get a FileNotFoundException where the class name itself is key to understanding what the problem is, just as much as the file name (in the message) is.
          Show
          David Smiley added a comment - I just reviewed this patch. One thing I'm seeing here that I usually consider to be a (minor) error is this pattern: catch (IOException e) { logger.error( "Couldn't persist core properties to {}: {}" , propfile, e.getMessage()); } Two things: I almost always pass the exception (variable e here) to the logger. Or in other cases involving throwing different exceptions, the parallel is forgetting to pass the exception as the cause. Was not doing this intentional? To put the details of the exception in the logged string, I suggest e.toString() as it includes the class as well as the message. Forgetting to do this can be confusing, like for example if you get a FileNotFoundException where the class name itself is key to understanding what the problem is, just as much as the file name (in the message) is.
          Hide
          Alan Woodward added a comment -

          Passing the exception itself to the logger generally ends up with a stacktrace being written out, which I don't think would be particularly useful here. But I like the idea of replacing e.getMessage() with e.toString(), will update. Thanks!

          Show
          Alan Woodward added a comment - Passing the exception itself to the logger generally ends up with a stacktrace being written out, which I don't think would be particularly useful here. But I like the idea of replacing e.getMessage() with e.toString(), will update. Thanks!
          Hide
          David Smiley added a comment -

          will update.

          Looking forward to that still

          Show
          David Smiley added a comment - will update. Looking forward to that still

            People

            • Assignee:
              Alan Woodward
              Reporter:
              Alan Woodward
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development