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.

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

@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.

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.

[elefeint]

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

[olavloite]

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

[elefeint]

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

[olavloite]

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.

[elefeint]

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.

[olavloite]

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.

[elefeint]

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

[olavloite]

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.

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.

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

@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.

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.

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.

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.

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