-
Notifications
You must be signed in to change notification settings - Fork 595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
datastore: use transaction object #1369
Conversation
What happens if there is an error? Do we rollback automatically? Does the user have to do this manually? |
Nope, and I'll add a to do item to make sure we rollback if the transaction.commit(function(err) {
// Behind the scenes, we see that this call fails.
// So before executing this callback, we should automatically rollback.
// What happens if the user didn't catch that part in the docs, and tries
// to rollback themselves?
if (err) {
transaction.rollback(...);
}
}); |
We just need to make sure to swallow any errors on the automatic rollback -- it will get an error:
|
/cc @jdimond |
c4ce6b9
to
41eec47
Compare
Added a commit to automatically rollback after a failed commit: 41eec47 |
// Rollback automatically for the user. | ||
self.rollback(function(err) { | ||
if (err) { | ||
err.message = [ |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
41eec47
to
f69cc97
Compare
To verify -- if an error happens during the BeginTransaction call, all attempted calls as part of that transaction will fail, correct? |
Are we worried users may try to do something like:
|
var transaction = datastore.transaction();
transaction.run(function(err) {
if (err) {
// BeginTransaction failed
}
}); Calling anything else from
That's a fair concern. We could return the transaction.run(function(err, tx) { /* transaction === tx */ });
datastore.transaction().run(function(err, transaction) {}); |
Just to be sure -- there are cases where someone might want to start a transaction, and then from outside the transaction execute a query of sorts, right? If there is no good reason to ever do: transaction.run( (err) => {
datastore.run(somethingElse);
transaction.commit();
}); then we should throw an error if people try to do something like that.... ? |
I think I'm a bit confused by your example. |
Sorry - I mean it as an example of "running another query from inside that block". Is there a time when that would make sense? That'd I'd |
Gotcha. In terms of a use case, I'm not sure. I don't think I'd block those operations, though. I'm not sure how, actually... choosing where a function can be executed or not: transaction.run(function (err) {
datastore.get(key, function(err, entity) {})
// ^-- need to detect this was executed inside of `transaction.run`
}) If this is what we want, maybe @callmehiphop and I can think of something clever. However, I think the user would expect if they have something that works in one place, it would work wherever they choose to put it. |
f69cc97
to
3f8c632
Compare
@callmehiphop PTAL |
3f8c632
to
5aa2c44
Compare
Fixes #633
The goal of this change is to give more visibility into the operations being performed when using transactions.
Before
After
The diff scariness doesn't reflect reality... I organized method names alphabetically. The code bodies themselves didn't change, just their location in the file & doc blocks.
@pcostell is this a good change in terms of usability?