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

Pass URL connection properties to avatica server

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.0-incubating
    • Fix Version/s: 1.5.0
    • Component/s: avatica
    • Labels:
      None
    • Flags:
      Patch

      Description

      This ticket proposes the ability to pass connection-creation-time properties (the "info") from the remote avatica driver to the avatica server.

      Use cases:

      • Phoenix properties like CurrentSCN and TenantId (and similar use-cases for a custom driver at the company I work for)
      • user and password properties (enables authentication, cfr. CALCITE-519 and CALCITE-643)

      See also discussion on mailing list:
      http://mail-archives.apache.org/mod_mbox/incubator-calcite-dev/201510.mbox/%3CCAAF1JdiZbRuMfo2CFRUoxPhBBf%3DyQ5T%2Bd%2B1ig1FzKnVDj8RGOA%40mail.gmail.com%3E

      Things to consider:

      • The remote driver might have its own properties (like "url") which should not be passed to the server. Remote driver is responsible for removing those.
      • The server might want to disallow setting some properties remotely, e.g. to not allow to connect to a different database.

      Currently the avatica server creates a default connection (though it is marked for removal). When using authentication, this is particularly annoying, since you need to start up the server with an url containing user and password.

      I will attach a patch, based on calcite 1.4, which adds the ability to pass properties as part of a new OpenConnectionRequest rpc call (a CreationConnetionRequest was proposed for different reasons in CALCITE-663). For the authentication reason mentioned, the default connection is also dropped. There is one JdbcMeta instance on the server side, but it manages a map of connections. Since connections were already created per client-side connection, this is not different than before, except that the connections are now created explicitly. Implicit connection creation has been dropped, we can look into auto-recreation of the connection as part of recovery (CALCITE-903).

      The somewhat hacky things in the patch are:

      • addition of AvaticaConnection.setId(): this is because I let the server decide on the connection id. This seemed more logical to me at first, though it can be easily reversed.
      • remote.Driver which creates an extra service instance to be able to do the OpenConnection request, while all other requests happen from within RemoteMeta.

      The patch also passes error messages back to the client (a simple improvement towards CALCITE-645).

      Because the calls for metadata (such as getColumns) now also have a connection id, fixing CALCITE-871 becomes easy and I have a patch ready for that as well.

      If the patch is acceptable, I can rework it for master (now it is for 1.4).

        Issue Links

          Activity

          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.5.0 (2015-11-10)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.5.0 (2015-11-10)
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/a63639b1 . Thanks for the PR, Bruno Dumon !
          Hide
          bruno Bruno Dumon added a comment -

          Thanks Josh Elser, looks good to me.

          Show
          bruno Bruno Dumon added a comment - Thanks Josh Elser , looks good to me.
          Hide
          elserj Josh Elser added a comment - - edited

          Hey Bruno Dumon, I updated your branch on top of master and addressed some of the feedback that Julian left previously.

          Let me know if it's helpful: https://github.com/joshelser/incubator-calcite/tree/CALCITE-912. It would be great to get these changes in before my PR that bumps the protobuf version gets merged in and breaks your code again .

          Edit: And also update CALCITE-645

          Show
          elserj Josh Elser added a comment - - edited Hey Bruno Dumon , I updated your branch on top of master and addressed some of the feedback that Julian left previously. Let me know if it's helpful: https://github.com/joshelser/incubator-calcite/tree/CALCITE-912 . It would be great to get these changes in before my PR that bumps the protobuf version gets merged in and breaks your code again . Edit: And also update CALCITE-645
          Hide
          bruno Bruno Dumon added a comment -

          Thanks for the feedback, I'll improve the patch and adjust it for master. Might not be before next week.

          The parameter to openConnection should not be a Properties (which is mutable) and I'm not sure whether the values should be strings. I think a Map<String, Object>.

          An advantage of <string, string> is that we're sure it can be serialized by jackson.

          Show
          bruno Bruno Dumon added a comment - Thanks for the feedback, I'll improve the patch and adjust it for master. Might not be before next week. The parameter to openConnection should not be a Properties (which is mutable) and I'm not sure whether the values should be strings. I think a Map<String, Object>. An advantage of <string, string> is that we're sure it can be serialized by jackson.
          Hide
          elserj Josh Elser added a comment -

          Generating statement IDs using Random.nextInt() is wrong.

          This was a recent change Julian made in CALCITE-906. Check out the change at:

          https://git1-us-west.apache.org/repos/asf?p=incubator-calcite.git;a=commitdiff;h=afcf3a6c;hp=4c7f5c20a04b4a4e736a16f801d8b5e6eded48cc

          Show
          elserj Josh Elser added a comment - Generating statement IDs using Random.nextInt() is wrong. This was a recent change Julian made in CALCITE-906 . Check out the change at: https://git1-us-west.apache.org/repos/asf?p=incubator-calcite.git;a=commitdiff;h=afcf3a6c;hp=4c7f5c20a04b4a4e736a16f801d8b5e6eded48cc
          Hide
          julianhyde Julian Hyde added a comment -

          First impressions on the first patch:

          • It seems right to allow a Meta to be shared among several connections, and therefore I agree with adding a ConnectionHandle to the various getXxx metadata methods.
          • Generating statement IDs using Random.nextInt() is wrong.
          • Removing 'final' from AvaticaConnection.id and .handle seems wrong.
          • Please don't remove the code in "getProcedures" etc because "RemoteMeta does not implement it either". One day RemoteMeta will implement it.
          • The error handling is useful - thanks!
          • The parameter to openConnection should not be a Properties (which is mutable) and I'm not sure whether the values should be strings. I think a Map<String, Object>.
          • Overall there are too many TODOs and hacks.
          Show
          julianhyde Julian Hyde added a comment - First impressions on the first patch: It seems right to allow a Meta to be shared among several connections, and therefore I agree with adding a ConnectionHandle to the various getXxx metadata methods. Generating statement IDs using Random.nextInt() is wrong. Removing 'final' from AvaticaConnection.id and .handle seems wrong. Please don't remove the code in "getProcedures" etc because "RemoteMeta does not implement it either". One day RemoteMeta will implement it. The error handling is useful - thanks! The parameter to openConnection should not be a Properties (which is mutable) and I'm not sure whether the values should be strings. I think a Map<String, Object>. Overall there are too many TODOs and hacks.

            People

            • Assignee:
              bruno Bruno Dumon
              Reporter:
              bruno Bruno Dumon
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development