Conversation

olavloite

Move the generic Connection API to the Spanner client library to make it re-usable. This refactor does the following:

  1. Move all non-JDBC classes, tests and resources from the JDBC driver to the Java client library. All files are placed in the com.google.cloud.spanner.connection package.
  2. Make the following classes and interfaces public in order to make them usable for the JDBC driver and other users of the package:
    1. Interface: com.google.cloud.spanner.connection.Connection
    2. Interface: com.google.cloud.spanner.connection.TransactionRetryListener
    3. Interface: com.google.cloud.spanner.connection.StatementResult
    4. Class: com.google.cloud.spanner.connection.ConnectionOptions
    5. Class: com.google.cloud.spanner.connection.ReadOnlyStalenessUtil
    6. Class: com.google.cloud.spanner.connection.StatementParser
    7. Enum: com.google.cloud.spanner.connection.TransactionRetryListener.RetryResult

All classes and interfaces that are marked as public have been marked with the @InternalApi annotation. The entire package is also marked as @InternalApi.

@olavloiteolavloite added do not mergeIndicates a pull request not ready for merge, due to either quality or timing.api: spannerIssues related to the googleapis/java-spanner API.labels Feb 14, 2020
@googlebotgooglebot added the cla: yesThis human has signed the Contributor License Agreement.label Feb 14, 2020
@olavloiteolavloite added theAdd this label to force Kokoro to re-run the tests.label Feb 14, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Feb 14, 2020
@meltsufin

@olavloite Can you please request @elefeint for the review as well?
She's leading the Spanner R2DBC effort.

@olavloite

@olavloite Can you please request @elefeint for the review as well?
She's leading the Spanner R2DBC effort.

I'd love to, but apparently she is not a member of this repository, so I can't request her review. I would certainly appreciate her input, though.

Choose a reason for hiding this comment

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

Looks great! I've brought it in and locally used it to parse R2DBC URLs.

I only have minor dependency and exceptions-in-comments comments.

</dependency>
<dependency>
<groupId>com.google.auth</groupId>
<artifactId>google-auth-library-credentials</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think google-auth-library-oauth2-http already brings this in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is correct, but this repository (and other repositories in googleapis) require all used dependencies to be explicitly declared. The maven dependency plugin is configured to fail the build if you don't. See the dependencies build log of the first commit: https://source.cloud.google.com/results/invocations/bd1192dc-11c9-43f4-8e36-f26d94ead93d/targets/cloud-devrel%2Fclient-libraries%2Fjava%2Fjava-spanner%2Fpresubmit%2Fdependencies/log

/**
* The retry was aborted by the {@link Connection} because of a concurrent modification. The
* transaction cannot continue and will throw an {@link
* AbortedDueToConcurrentModificationException}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above -- AbortedDueToConcurrentModificationException too tied to JDBC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The text in the comment was not entirely correct. The exception is thrown by the connection API and then re-thrown by the JDBC driver as a com.google.cloud.spanner.jdbc.JdbcAbortedDueToConcurrentModificationException. I've updated the text and removed all references to JDBC.

* silently handle any {@link AbortedException}s by internally re-acquiring all transactional locks
* and verifying (via the use of cryptographic checksums) that no underlying data has changed. If a
* change to the underlying data is detected, then an {@link
* AbortedDueToConcurrentModificationException} error will be thrown. If your application already
Copy link
Contributor

Choose a reason for hiding this comment

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

The exception hierarchy is Jdbc-specific. It's just mentioned in a comment (and pre-rename -- it's JdbcAbortedDueToConcurrentModificationException now), so it's not very important, but it might be better to avoid mentioning a JDBC-specific exception in Connection API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated this comment as well. Handling aborted transactions is done by the connection API, and the JDBC driver is just re-throwing these exceptions in a JDBC-specific class.

* track of a running SHA256 checksum of all {@link ResultSet}s that have been returned from Cloud
* Spanner. If the checksum that is calculated during an internal retry differs from the original
* checksum, the transaction will abort with an {@link
* AbortedDueToConcurrentModificationException}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above -- AbortedDueToConcurrentModificationException too tied to JDBC.

Copy link
Collaborator Author

@olavloite olavloite Feb 15, 2020

Choose a reason for hiding this comment

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

No, I don't agree with that. Aborted transactions are handled by the connection API and could be relevant for any framework built on top of it. If the framework does not have some kind of automatic retry-loop for transactions, this feature of the connection API should be used to prevent the user from being confronted with aborted errors.
I've updated the comment to correctly link to the correct exception.

@clifffrey

I just want to say that I'm comfortable with this overall approach, and excited for the connection api to be made available for other libraries besides JDBC.

I would love for there to be something explicit about the level of support (and API non-breaking-ness) we are promising for this API. i.e. is every method exposed in the public interface going to be supported until the next major version? Do we want to claim less-than-that to start out with? It just seems important that we don't accidentally make this a fully supported/unchanging API without being clear.

Choose a reason for hiding this comment

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

Thank you! Looks good from my side.

To Cliff's point, perhaps we could have a README in the connection package explaining that this is internal only, "so please do not use this API by itself", and then relax this constraint over time if necessary?

I am out on Monday, so I'll get back to this on Tuesday.

Choose a reason for hiding this comment

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

@olavloite Can you explain the decision for not moving the connection API to a separate Maven module in the repo?

@olavloite

@olavloite Can you explain the decision for not moving the connection API to a separate Maven module in the repo?

@meltsufin The honest answer is that I didn't think of that possibility as well :-). I automatically interpreted the suggestion to 'add it to the client library repository' as 'add it to the client library module'.

That being said, I would say that the following argument favors putting them together:

  • Ease of versioning: The connection API only relies on the public API of the client library, so it could just have a dependency on the client library module. The two are however in real life somewhat more interconnected, and there have been changes in the past where a version of the connection API (or actually; the JDBC driver) depended on a specific minimum version of the client library. Users who would exclude the transitive client library dependency brought in by the connection API and import their own version of the client library would in those cases run into subtle bugs. (Of course one could argue that anyone who excludes a transitive dependency is responsible for knowing what they're doing.)

And the following would argue against putting them together:

  • It adds build time to the client library.
  • Anyone who imports the client library automatically also imports the connection API. This might confuse users as to which API to use.

I personally don't really feel very strongly either way, and I'm certainly open to additional arguments and views on this.

@meltsufin

Regarding versioning, users should really be using the Spanner BOM the ensure compatibility between the Connection API and the Client Lib.

In addition to the benefits of separating into a separate module that you already outlined, another one is that it will ensure that the Client Library does not accidentally rely on something in the Connection API.

I guess the decision ultimately depends on how decoupled you want the Connection API to be from the client library.

@skuruppu

Thank you! Looks good from my side.

To Cliff's point, perhaps we could have a README in the connection package explaining that this is internal only, "so please do not use this API by itself", and then relax this constraint over time if necessary?

I am out on Monday, so I'll get back to this on Tuesday.

Could we also add the @internalapi annotation to make it explicit in the code?

Edit: I realize that this might not work because it would mean that the JDBC driver can't use it.

@skuruppu

Just my 2 cents on the two modules vs one module discussion. I feel that the connections API is strongly tied to the client library implementation and I can't think of any use cases where someone might want to use a different version of the connections API to that of the client library. So in terms the ease of us managing the modules/version and for users to not get into a confusing versioning situation, I'm in favor of having both being in the same module.

@olavloiteolavloite force-pushed the add-connection-api branch from 0f5ff10 to 969003d Compare March 5, 2020 14:21
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 5, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 5, 2020
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 5, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Mar 5, 2020
@olavloiteolavloite removed the do not mergeIndicates a pull request not ready for merge, due to either quality or timing.label Mar 6, 2020
@skuruppu

I haven't reviewed this line by line since this is PR is just moving code. @olavloite please feel free to merge.

@olavloiteolavloite force-pushed the add-connection-api branch from 969003d to 79f6678 Compare April 2, 2020 18:47
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@olavloiteolavloite added the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests.label Apr 3, 2020
@olavloiteolavloite merged commit d617fb6 into master Apr 4, 2020
@olavloiteolavloite deleted the add-connection-api branch April 4, 2020 08:36
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
* feat: add support for QueryOptions

* fix: review comments

* deps: temporarily update because of build and test errors

* deps: update to released versions

* fix: use grpc 1.27.2

* fix: fix invalid query hint in IT

Co-authored-by: skuruppu <[email protected]>
Sign up for free to join this conversation on . Already have an account? Sign in to comment
api: spannerIssues related to the googleapis/java-spanner API.cla: yesThis human has signed the Contributor License Agreement.
None yet

Successfully merging this pull request may close these issues.