Details

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

      Description

      Now Calcite no longer supports JDK 1.6 we can use AutoCloseable if we want to. It is more appropriate interface for releasing resources on close because it is not so tightly associated with I/O. (Closeable.close throws IOException.) Most, if not all, uses of Closeable in Calcite should instead use AutoCloseable.

        Activity

        Hide
        jnadeau Jacques Nadeau added a comment -

        It would be good to switch to the try-with-resources pattern also as part of this change.

        Show
        jnadeau Jacques Nadeau added a comment - It would be good to switch to the try-with-resources pattern also as part of this change.
        Hide
        julianhyde Julian Hyde added a comment - - edited

        I think that would be dangerous. I tried converting SplunkConnectionImpl.connect https://github.com/apache/calcite/blob/master/splunk/src/main/java/org/apache/calcite/adapter/splunk/search/SplunkConnectionImpl.java#L120 to use try-with-resources and realized that I was subtly changing the behavior BufferedReader.close threw an exception. I don't want to introduce bugs, so I backed out that particular change.

        The code change for this bug is very small and (I believe) safe. But because it widens Closeable to AutoCloseable it gives new opportunities to use try-with-resources.

        But yes, we should encourage people to use try-with-resources in new code, and convert old code when it makes sense.

        Show
        julianhyde Julian Hyde added a comment - - edited I think that would be dangerous. I tried converting SplunkConnectionImpl.connect https://github.com/apache/calcite/blob/master/splunk/src/main/java/org/apache/calcite/adapter/splunk/search/SplunkConnectionImpl.java#L120 to use try-with-resources and realized that I was subtly changing the behavior BufferedReader.close threw an exception. I don't want to introduce bugs, so I backed out that particular change. The code change for this bug is very small and (I believe) safe. But because it widens Closeable to AutoCloseable it gives new opportunities to use try-with-resources. But yes, we should encourage people to use try-with-resources in new code, and convert old code when it makes sense.
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/9c86556f .
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.6.0 (2016-01-22).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.6.0 (2016-01-22).

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            julianhyde Julian Hyde
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development