Make conditional vs unconditional updates clearer #48359
Conversation
hugoShaka
added
no-changelog
hugoShaka
force-pushed
the
hugo/clarify-unconditional-updates
branch
from
9bd32cd to
2e081a1
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| // UpdateResource updates an existing resource. | ||
| func (s ServiceWrapper[T]) UpdateResource(ctx context.Context, resource T) (T, error) { | ||
| // UnconditionalUpdateResource updates an existing resource without checking the provided resource revision. | ||
| // This is faster and cheaper than ConditionalUpdateResource but can cause data loss on concurrent writes. |
There was a problem hiding this comment.
I don't think this description is particularly useful:
- the backend
Updateoperation by itself itself is not really faster or cheaper thanConditionalUpdate - the operation doesn't cause data loss, the usage pattern does
- this description doesn't describe when it should be used (pretty much never), what the tradeoffs are, and what the alternative is
| adapter, err := s.service.UpdateResource(ctx, newResourceMetadataAdapter(resource)) | ||
| return adapter.resource, trace.Wrap(err) | ||
| } | ||
|
|
||
| // ConditionalUpdateResource updates an existing resource if the provided | ||
| // resource and the existing resource have matching revisions. | ||
| // This is safer than UnconditionalUpdateResource but more expensive and might not suit high-load scenarios. |
There was a problem hiding this comment.
More expensive than what? In a lot of backends the cost of the storage operation itself is pretty much the same, it's the usage pattern around optimistic locking that's more expensive than blindly upserting a value.
Without expanding too much in the docs here (unless you think it's useful) we could just link to https://github.com/gravitational/teleport/blob/master/rfd/0126-backend-migrations.md#optimistic-locking and https://github.com/gravitational/teleport/blob/master/rfd/0153-resource-guidelines.md#update-1 . WDYT?
hugoShaka
force-pushed
the
hugo/clarify-unconditional-updates
branch
from
64866cb to
e8edf1d
Compare
public-teleport-github-review-bot
bot
removed the request for review
from kimlisa
|
@hugoShaka See the table below for backport results.
|
When implementing AU services we mistakenly used unconditional updates instead of conditional updates.
This PR renames
UpdateResourceintoUnconditionalUpdateResourceto make it clearer that this might be unsafe and there's aConditionalUpdateResourceoption.