Update GORM insecure example with app retry loop #5084
Conversation
|
This change is |
|
Ben, I think I've got this code sample working properly using the recommended Based on my tests with a local cluster it is working properly with that update: casting GORM error to At your convenience do you mind testing it and verifying that it works as it should? Once it looks good to you I'll clean it up by removing the commented out retry stuff, and add in the secure example boilerplate, etc. |
| var fromAccount Account | ||
| var toAccount Account | ||
|
|
||
| db.First(&fromAccount, 1) |
There was a problem hiding this comment.
These calls must be inside the transaction. Any time the transaction retries, we need to re-query the starting balances. (This applies to the other language examples too).
There was a problem hiding this comment.
Fixed these by moving them inside transferFunds, which is now a function argument to a runTransaction function very similar to the Python examples.
| } | ||
|
|
||
| var maxRetries = 3 | ||
| for retries := 0; retries <= maxRetries; retries++ { |
There was a problem hiding this comment.
In general, you'd want to refactor the retry loop into a higher-order function instead of inlining it in every function that needs a transaction (where supported by the language). I think it's probably a good idea to do that in our examples.
There was a problem hiding this comment.
Done. It's pretty close to the Python version but may need some updates to be idiomatic Go. PTAL.
| } | ||
|
|
||
| txn := db.Begin() | ||
| // The statement below is used for forcing transaction retries |
There was a problem hiding this comment.
Explain a little more about what's going on: The first statement in a transaction can be retried transparently on the server, so we need to add a dummy statement so that our force_retry statement isn't the first one.
There was a problem hiding this comment.
Updated with this explanation.
| // The below statement is used for forcing transaction | ||
| // retries for verifying the retry loop logic. It is not | ||
| // necessary for production code. | ||
| // `SELECT crdb_internal.force_retry('1s'::INTERVAL)`, |
There was a problem hiding this comment.
It's stylistically weird to insert this at this point. I'd put this in its own txn.Exec statement after the txn.Exec for SELECT now(). This will also force a refactoring to put the error handling in one place (which again would mean making a runTransaction higher-order function)
There was a problem hiding this comment.
This is all refactored into the runTransaction HOF now.
| } else { | ||
| // Happy case. All went well, so we commit and break out | ||
| // of the retry loop. | ||
| txn.Commit() |
There was a problem hiding this comment.
txn.Commit can return an error which must be handled. In particular it can return a retry error.
There was a problem hiding this comment.
Updated to check for 40001 errors on txn.Commit() and continue the retry loop if they're encountered.
| // retries for verifying the retry loop logic. It is not | ||
| // necessary for production code. | ||
| // `SELECT crdb_internal.force_retry('1s'::INTERVAL)`, | ||
| `UPSERT INTO accounts (id, balance) VALUES |
There was a problem hiding this comment.
Users of an ORM don't generally want to write SQL queries by hand like this. Idiomatic use of GORM would be to either modify the Account objects and call Save on them or use gorm's Update method.
There was a problem hiding this comment.
Updated to modify the Account objects and call Save on them.
... and update the surrounding descriptive text to match the new code. Addresses #5035.
rmloveland
force-pushed
the
20190710-app-retries-golang
branch
from
6168247 to
ff98e13
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)
_includes/v19.1/app/insecure/gorm-sample.go, line 41 at r3 (raw file):
} // Used to force a transaction retry. Can only be run as the // 'root' user.
FYI I just filed cockroachdb/cockroach#39243 to change this.
There was a problem hiding this comment.
Thanks for the review Ben!
Reviewable status:
_includes/v19.1/app/insecure/gorm-sample.go, line 41 at r3 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
FYI I just filed cockroachdb/cockroach#39243 to change this.
Good to know, thanks.
... and update the surrounding descriptive text to match the new code.
Includes updates based on review comments in #5049, which for some reason stopped accepting updates.
Addresses #5035.