Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1482

Resource leak on creation of CassandraSchema

    Details

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

      Description

      Found via the following Coverity scan

      *** CID 138163:  Resource leaks  (RESOURCE_LEAK)
      /cassandra/src/main/java/org/apache/calcite/adapter/cassandra/CassandraSchema.java: 115 in org.apache.calcite.adapter.cassandra.CassandraSchema.<init>(java.lang.String, java.lang.String, java.lang.String, java.lang.String, org.apache.calcite.schema.SchemaPlus, java.lang.String)()
      109         } catch (Exception e) {
      110           throw new RuntimeException(e);
      111         }
      112         this.parentSchema = parentSchema;
      113         this.name  = name;
      114
      >>    CID 138163:  Resource leaks  (RESOURCE_LEAK)
      >>    Ignoring resource created by "org.apache.calcite.runtime.Hook.TRIMMED.add(this.new org.apache.calcite.adapter.cassandra.CassandraSchema.1())" leaks it.
      115         Hook.TRIMMED.add(new Function<RelNode, Void>() {
      116           public Void apply(RelNode node) {
      117             CassandraSchema.this.addMaterializedViews();
      118             return null;
      119           }
      120         });
      

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.11.0 (2017-01-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).
        Hide
        michaelmior Michael Mior added a comment -

        Sorry about that. I know I've made the same mistake before and I thought I
        remembered correctly this time.

        https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=dd3782e73c7ef16abeeb7bb1a06f157a68c5fce7

        Show
        michaelmior Michael Mior added a comment - Sorry about that. I know I've made the same mistake before and I thought I remembered correctly this time. https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=dd3782e73c7ef16abeeb7bb1a06f157a68c5fce7
        Hide
        julianhyde Julian Hyde added a comment -

        Michael Mior, When you commit a fix, can you change the JIRA status to FIXED not CLOSED (we close when we make the release) but do set the fixVersion. Also, paste a link to the commit into the JIRA case.

        Show
        julianhyde Julian Hyde added a comment - Michael Mior , When you commit a fix, can you change the JIRA status to FIXED not CLOSED (we close when we make the release) but do set the fixVersion. Also, paste a link to the commit into the JIRA case.
        Hide
        michaelmior Michael Mior added a comment -

        Done. Sorry for the delay, just wanted to run the integration tests first to make sure nothing was amiss.

        Show
        michaelmior Michael Mior added a comment - Done. Sorry for the delay, just wanted to run the integration tests first to make sure nothing was amiss.
        Hide
        julianhyde Julian Hyde added a comment -

        +1 Michael Mior please go ahead and commit

        Show
        julianhyde Julian Hyde added a comment - +1 Michael Mior please go ahead and commit
        Hide
        maryannxue Maryann Xue added a comment -

        +1 for the short term fix. And this is exactly what we are doing in Phoenix right now.
        The reason why we have to use hooks for Phoenix is that we only want to register those secondary indices that can be potentially used by the query, otherwise we would end up going through the entire Phoenix metadata to register all existing secondary indices. Optimally we would like to have the "pull" model vs. this "push" model, and to not have to go through any registration process into a static MaterializationService instance at all. For example, we could have an interface called getMaterializations(CalciteSchema rootSchema) so that the adapter (Phoenix or Cassandra or anything else) can provide a list of available materialization definitions and those definition entries will be compiled by Calcite into constructs that can be used by the query planner.
        I guess the reason why we have MaterializationService as long-standing instance is to be able to define and populate materializations. So I'm thinking to actually decouple these two things, definition and population, and to have MaterializationService dedicated to population only.

        Show
        maryannxue Maryann Xue added a comment - +1 for the short term fix. And this is exactly what we are doing in Phoenix right now. The reason why we have to use hooks for Phoenix is that we only want to register those secondary indices that can be potentially used by the query, otherwise we would end up going through the entire Phoenix metadata to register all existing secondary indices. Optimally we would like to have the "pull" model vs. this "push" model, and to not have to go through any registration process into a static MaterializationService instance at all. For example, we could have an interface called getMaterializations(CalciteSchema rootSchema) so that the adapter (Phoenix or Cassandra or anything else) can provide a list of available materialization definitions and those definition entries will be compiled by Calcite into constructs that can be used by the query planner. I guess the reason why we have MaterializationService as long-standing instance is to be able to define and populate materializations. So I'm thinking to actually decouple these two things, definition and population, and to have MaterializationService dedicated to population only.
        Hide
        julianhyde Julian Hyde added a comment -

        Here is Michael Mior's proposed fix: https://github.com/michaelmior/calcite/commit/aada6f33de92901c6df9be9f0d890903332bf8e4

        It looks good to me, at least for the short term. Maryann Xue, Can you please review, and maybe suggest a better long-term solution?

        Show
        julianhyde Julian Hyde added a comment - Here is Michael Mior 's proposed fix: https://github.com/michaelmior/calcite/commit/aada6f33de92901c6df9be9f0d890903332bf8e4 It looks good to me, at least for the short term. Maryann Xue , Can you please review, and maybe suggest a better long-term solution?

          People

          • Assignee:
            michaelmior Michael Mior
            Reporter:
            michaelmior Michael Mior
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development