Datastore: cannot create more than one entity while running inside a transaction

[silvolu]

When I try to create the second entity I receive the following error:
Uncaught Error: The referenced transaction has expired or is no longer valid.

Happens because DatastoreRequest.prototype.save commits regardless of whether we are in a transaction or not.

DatastoreRequest.prototype.delete does the same, which means that I cannot have 2 deletes, 1 save and 1 delete....

[stephenplusplus]

What should it do? Not make the commit call until done() is called? That would mean we build a mutation object in memory up until that point with their changes? Our API would change quite a bit if that's the case, e.g. transaction.save() wouldn't be async.

[rakyll]

Typically, you use transactions for reads, you don't make a transactional commit that mutates the data. Once you save, you actually commit the transaction and therefore it's finalized. We should make the transactional save a private API maybe.

[rakyll]

Btw, the AppEngine runtime libraries don't support transactional saves.

[silvolu]

What should it do? Not make the commit call until done() is called? That would mean we build a mutation object in memory up until that point with their changes?

That's what gcloud-python does: add to mutation and then commit only when explicitly called or implicitly called when exiting a transactional context.

Btw, the AppEngine runtime libraries don't support transactional saves.

In DB there are a couple of examples that create 2 entities inside a transaction, in the Using transactions sections (sorry I can get a better link to it).

I'm emailing the API team to check what they think.

[Alfus]

It is very important to be able to change two entities transactionally. The Google Cloud Datastore API was design specifically to require that mutation be buffered and only committed when done.

Why would this remove the ability to make transactions async?

[stephenplusplus]

Our API looks like: transaction.save() and transaction.delete(). These are currently async because they are making an API call each time. If we can only make one commit call per transaction when the user calls done(), then those save and delete calls aren't doing anything async.

[stephenplusplus]

To clarify, that's the API a user has after already starting the transaction, which itself is async: dataset.runInTransaction(callback) http://googlecloudplatform.github.io/gcloud-node/#/docs/v0.10.0/datastore/dataset?method=runInTransaction

[Alfus]

Ya, those do not need to be async, they should just buffer the mutations (likely using a map from key -> null or value, to avoid multiple mutations on the same key) and done() should call commit even if there are no mutations to commit.

[stephenplusplus]

Sounds good to me. Thanks!

Should we keep the same API, just ditch the callback?:

dataset.runInTransaction(function(transaction, done) {
  var key = dataset.key(['Company', 123]);

  transaction.get(key, function(err, entity) {});

  transaction.save({
    key: key,
    data: {
      rating: 10
    }
  });

  transaction.delete(key);

  done();
}, function(err) {});

In the above scenario, the entity marked by key would be deleted, as that was the last operation, correct? If it was the other way around: delete() before save(), we would ignore the delete and commit the save?

** edit ** Maybe the better handling is to throw on the second operation of any kind on a key.

[Alfus]

Yes, the last operation 'wins.'
I am not saying you should do it, but it might be useful for the developer if they could specify a callback even though save and delete are not async to make coding inside and outside a transaction look the same. In this case the callback would just be called immediately (or maybe less error prone, right after the transaction function finished, does done() block on all outstanding async calls?).

Another interesting thing to consider is, what if they call save on an entity with an incomplete key? That should either be an error or cause allocateIds to implicitly be called (making the save operation async again). Not making this an error would be very useful, especially for the case when a user wants to store two entities one containing a reference to the other but the other key is incomplete. Do you guys do implicit batching? (for example batching up all the async gets sending a single RPC for them)

[silvolu]

Yes, the last operation 'wins.'

I was worried about cases like the first example from @stephenplusplus :

transaction.save({
  key: key,
  data: {...}
});
transaction.delete(key);

Where we would end up sending a delete for a key that doesn't exist, but I guess the transaction would just fail with a meaningful error?

I am not saying you should do it, but it might be useful for the developer if they could specify a callback even though save and delete are not async to make coding inside and outside a transaction look the same. In this case the callback would just be called immediately (or maybe less error prone, right after the transaction function finished, does done() block on all outstanding async calls?).

I think the way to do this would be call all the callback right after the transaction function finished, but it would become a bit messy to keep track of which arguments to pass to which callback.
I think it's better to just document that when running inside a transaction, there will be only on callback called at the end, and the keys of all the saved entities will be passed to the callback.

Another interesting thing to consider is, what if they call save on an entity with an incomplete key?
We currently add such entities to insert_auto_id. That should take care of it?

Do you guys do implicit batching? (for example batching up all the async gets sending a single RPC for them)

No, the user can explicitly batch by passing an array of keys to get.

[Alfus]

Deleting an entity that doesn't exist is currently not an error (it is a noop).

For the allocate ids case, consider:

transaction.save({
  key: incomplete_key1,
  data: {...}
}
transaction.save({
  key: key2,
  data: {ref: incomplete_key1, ...}
})

the only way the second save can refer to the previous key is if an id has been allocated for that key. This means that the caller must manually call allocate ids, or the implementation must provide the completed key in a callback for the save operation. What happens for a save outside of a transaction in this case?

Seems like there is some nice room to optimize with implicit batching if everything is async.

[stephenplusplus]

Friendly ping for updates on the direction we wish to take this.

[silvolu]

For now, just focus on fixing the main issue (create/delete multiple keys) with the 'last operation wins' approach for operations on the same key.
We'll tackle batching/auto allocate ids in separate issues.