feat: support Savepoint

[olavloite]

Add support for emulated Savepoints that are now supported in the client library.

[thiagotnunes]

LGTM

[arpan14]

Any reason we are preferring static methods for instantiation instead of adding constructors for creating named and unnamed instances? Wouldn't the constructor be a more natural way for creating instance instead of static methods?

[olavloite]

This gives more readable code where the savepoints are created, especially as the behavior of named and unnamed savepoints are slightly different. Internally, we always need to create savepoints with a name, as that is the only thing that Cloud Spanner supports. This way we make clear that one is a named savepoint, and the other is unnamed, even though both internally have a name.

[arpan14]

Suggestion : It might be more readable to track named vs un-named through an explicit boolean variable say isNamed . Using the the ID's state to determine this seems like an over-use of ID's value beyond what its meant for. For example - If for any reason we change this ID's type from integer to String, it ideally shouldn't impact the logic to determine named vs un-named. But in this case we will need to modify the logic.

[olavloite]

I don't feel it's really necessary given the size of the class. Also; changing the type of ID from integer to anything else will not be possible, as the type is specified by the JDBC API (that is; getSavepointId must return an int).

[arpan14]

Have we intentionally skipped checkClosed(); validation in this method? If yes, I didn't get why we won't need the check here.

[olavloite]

Not really intentionally. Added it to be consistent with the other methods.

[arpan14]

1. Why is this method being added to an implementation when the interface Savepoint is not defining any such method?
2. Why are we prefixing this with internal ? Wouldn't it be better to call this getName which acts like a getter method for existing member name?
3. Why are we adding this method when there is already getSavepointName() method?

[olavloite]

We need a separate method from getSavepointName() because:

1. JDBC specifies that getSavepointName() must throw an error if it is called on an unnamed savepoint.
2. We internally need to get the name of a savepoint in order to set it on the underlying connection. For an unnamed savepoint, this will be a generated name that is only accessible internally in the package, and not to the outside world.
3. internalGetSavepointName() is the internal (package-private) version of the getSavepointName() method that returns the name regardless whether the savepoint is named or unnamed. This is used internally to set the savepoint name on the underlying connection.

[arpan14]

Thanks for the explanation.

[arpan14]

What's your take on inversion of control ? Are you intentionally using JdbcSavepoint since you require the static methods and the internalGetSavepointName() method?

Usually with interfaces we don't expose the implementation class at time of dependency injection. Over here it's not really dependency injection, but would it not be better to use just Savepoint interface instead of referencing JdbcSavepoint class?

[olavloite]

We need to use the JdbcSavepoint class instead of the Savepoint interface here, because we need access to the internalGetSavepointName() method. We cannot use the Savepoint#getSavepointName() method, because that method will throw an error for unnamed savepoints. The 'throw-an-error' behavior is part of the JDBC specification, so it is not something that we can choose to implement differently.

[arpan14]

Similar comments on usage of JdbcSavepoint class instead of Savepoint interface as above methods.

[olavloite]

Same as above: We need access to the internalGetSavepointName() method, as the Savepoint#getSavepointName() could throw an error if the given savepoint is an unnamed savepoint.