Use try-with-resources for Transactions about google-cloud-java

[aozarov]

Yes, the reason that we didn't make Transaction Closable is because close does not have enough context (as oppose to Python with clause) about success/failure of the operation.
I think having the need to explicitly commit but rather automatically rollback (if not committed) is not intuitive/expected and I would like to get some more feedback/opinions before going ahead with it.
For now we provided the DatastoreService#runInTransaction convenient method.

[eamonnmcmanus]

I agree with Arie that it would be surprising for Transaction.close() to mean "abort the transaction if it is not committed". What you would want instead would be for it to mean "commit the transaction unless there was an exception, in which case abort it". But unfortunately there's no way for the close() method to know whether it is being invoked because of an exception or because execution of the try block completed normally.

I think I would be in favour of having only the DatastoreService.runInTransaction method so that it is impossible to accidentally abandon a transaction due to an exception. The argument to TransactionCallable would then need to be a Transaction, which is probably better than a DatastoreReaderWriter anyway? The resulting code would be slightly more verbose on Java 7 or before, but quite succinct on Java 8 with lambdas:

result = dataStore.runInTransaction((t) -> {
  t.query()...t.put()...;
  return something;
});

[aozarov]

I agree that passing Transaction instead of DatastoreReaderWriter to DatastoreService.runInTransaction sounds nicer but it has the side effect of also including a way
to manually/explicitly commit or rollback (the main 2 methods that Transaction adds to DatastoreReaderWriter).

Though having an explicit commit/rollback option in the callback could be seen as advantage/feature, I feel that it may lead to more confusing user code (where the callback code can't tell for sure if it is still active) and thought we should better avoid it.

Few possible options:

1. Leave it as is.
2. Replace callback param with a new TransactionCallback interface (similar to Transaction but without a way to explicitly commit/rollback).
3. Replace callback param with Transaction (and allow callback to explicitly commit/rollback).

Thoughts?

[jboynes]

You need transactions to fail safe i.e. roll back by default. The user must specifically commit the work and know that the commit succeeded so that other work can rely on it. The nasty path is where the VM itself raises an Error (which can happen at any time) which must result the work getting rolled back.

The simplest way to ensure that behaviour is to have the user explicitly commit their work. The user can then assume that any error that prevented the commit operation completing successfully resulted in no work being applied.

For reference, where the JDBC spec says it is "implementation-defined" (doh!) whether Connection#close() commits or rolls back, the drivers from MySQL, Postgres, DB2 (non-mainframe) and SQLServer all roll back by default whereas only Oracle and DB2 (mainframe) commit. Similarly a UserTransaction in JavaEE will always rollback unless it is specifically committed.

[jboynes]

Based on @eamonnmcmanus comment, Java 8's lambdas would make this intuitive:

interface DataStore {
  /** Simple way to run work in a transaction. */
  <ResultT> ResultT runInTransaction(Supplier<ResultT> work);

  /** Run work in a transaction with ability to manually control outcome. */
  <ResultT> ResultT runInTransaction(Function<Transaction, ResultT> work);
}

The Transaction interface would only need to add one method for transaction control:

interface Transaction extends DatastoreReaderWriter {
  void setRollbackOnly();
}

which would would like the same method in javax.transaction.UserTransaction

Work would be automatically committed if the function returned normally, or would be rolled back if the block threw a Throwable. Per Java8 functional APIs, the work would not be able to throw a checked Exception.

To run this on Java 7, we can backport the Supplier and Function interfaces. I realize equivalents exist in Guava but I suggest we avoid using Guava classes in the API.

[garrettjonesgoogle]

This has been added to our feature backlog: https://github.com/GoogleCloudPlatform/google-cloud-java/wiki/Feature-backlog . This issue will be closed but is linked in the backlog and can continue to be used for comment and discussion.