Skip to content

Minor JDBC driver improvements#6979

Merged
electrum merged 5 commits intoprestodb:masterfrom
electrum:jdbc-driver
Feb 28, 2017
Merged

Minor JDBC driver improvements#6979
electrum merged 5 commits intoprestodb:masterfrom
electrum:jdbc-driver

Conversation

@electrum
Copy link
Contributor

@electrum electrum commented Dec 29, 2016

Fixes #6929

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 minor comments/questions:

  1. Will this return 0 for every snapshot version when getDatabaseMinorVersion() is called? If so, that seems like a pain for us or anybody doing testing.
  2. For end users, it seems likely that the only reason you'd hit this is because the presto versioning scheme has radically changed and the driver version parsing code is out of date. It seems like a good bet that the user needs to update the driver. How about throwing an SQLException containing the full version string like "Driver version x.y could not parse server version string '%s'. Try updating the driver"

Please feel free to ignore if this is spec-compliant behavior as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. If the driver JAR is built from Maven, it will work correctly, since the snapshot JAR will have the correct manifest. I just tested this out (and fixed a bug). The version for snapshots is a bit weird -- it's the output of git describe HEAD. So, for this branch on 0.162-SNAPSHOT, the version is 0.161-115-gea22394b5 (and that depends on having fetched the tags to the local Git repo). Thus, you get 0/161 for major/minor, and the full string for getDriverVersion().
  2. I considered that, but it will break for stuff like presto.version=testversion that we have in the development config.properties file. I'd rather be conservative for now.

Copy link
Contributor Author

@electrum electrum Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized my comment for #1 is actually about the JDBC driver version, not the server version. However, the answer is nearly the same. The server version is determined the same way, from the package manifest, unless explicitly overridden with presto.version.

@ebd2
Copy link
Contributor

ebd2 commented Jan 3, 2017

Looks basically reasonable to me % usability nit. Somebody who is more up to date with what is supported should probably look over those changes.

Copy link
Contributor

@dain dain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@electrum electrum closed this Feb 28, 2017
@electrum electrum deleted the jdbc-driver branch February 28, 2017 23:42
@electrum electrum merged commit c57e7a3 into prestodb:master Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants