Conversation
@olavloite Can you please request @elefeint for the review as well? |
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. |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}. |
There was a problem hiding this comment.
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.
olavloite Feb 15, 2020 •edited
LoadingUh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@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:
And the following would argue against putting them together:
I personally don't really feel very strongly either way, and I'm certainly open to additional arguments and views on this. |
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. |
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. |
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. |
0f5ff10
to 969003d
Compare I haven't reviewed this line by line since this is PR is just moving code. @olavloite please feel free to merge. |
969003d
to 79f6678
Compare * 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]>
🤖 I have created a release \*beep\* \*boop\* --- ## [1.15.0](https://www..com/googleapis/java-spanner-jdbc/compare/v1.14.0...v1.15.0) (2020-03-24) ### Features * add support for QueryOptions ([googleapis#76](https://www..com/googleapis/java-spanner-jdbc/issues/76)) ([b3f4cf7](https://www..com/googleapis/java-spanner-jdbc/commit/b3f4cf7852a2fd5f22660cc3f25a6253b9a118ab)) ### Dependencies * update spanner.version to v1.52.0 ([googleapis#95](https://www..com/googleapis/java-spanner-jdbc/issues/95)) ([cdf9d30](https://www..com/googleapis/java-spanner-jdbc/commit/cdf9d30e8ca387d87a6ffe00fa09818d135547f4)) --- This PR was generated with [Release Please](https://.com/googleapis/release-please).
Move the generic Connection API to the Spanner client library to make it re-usable. This refactor does the following:
com.google.cloud.spanner.connection
package.com.google.cloud.spanner.connection.Connection
com.google.cloud.spanner.connection.TransactionRetryListener
com.google.cloud.spanner.connection.StatementResult
com.google.cloud.spanner.connection.ConnectionOptions
com.google.cloud.spanner.connection.ReadOnlyStalenessUtil
com.google.cloud.spanner.connection.StatementParser
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
.