sql,kv: replace InitPuts with CPuts #138707
Draft
+255
−613
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit switches the usage of `InitPut` KV request when inserting a new sql liveness session from `FailOnTombstones=true` to `FailOnTombstones=false`. The contract of the `Insert` method is that the caller should never provide previously used session ID which is achieved by generating a random V4 UUID, so the probability of ever generating a duplicate is miniscule. The usage of `FailOnTombstones=true` option provides additional protection if the same session ID is reused within the GC TTL of the relevant range (because tombstones would get removed on MVCC garbage collection and/or compactions in pebble). This usage was introduced in 686f323 where we switched from SQL INSERT statement - which behaved similar to `FailOnTombstones=false`. My main motivation behind this change is that this is the only place where we use `FailOnTombstones=true` option of the InitPut, and we're about to deprecate the InitPuts in favor of CPuts, yet CPuts don't have this option which would make the deprecation process more involved. Also I think the current handling of this scenario by the code is partially broken. Namely, if previously used session ID is provided while there is a tombstone on the relevant key, the ConditionFailedError would be returned, and the loop in `Instance.createSession` would exceed the retry duration because on this error type we do not reset `session.id` field. This will result in not being able to create a session by the SQL instance, so it will fatal the process. On a quick search via Glean we never encountered this fatal before though. With the change in this commit in the case of inserting the previously used session ID the behavior is undefined (I think one possible scenario is that other SQL instances won't be able to see this new instance because they have cached the session ID in the deadSessions cache, so the new instance won't be used for distributed queries; there are probably other consequences too). Still, it seems reasonable to me to rely on randomly-generated UUIDs never repeating without additional - but only temporary - protection of `FailOnTombstones`. Release note: None
We recently removed the only usage of `FailOnTombstones` option of the InitPut, so we can remove the client side support for it completely now. Release note: None
CPut with empty `expValue` parameter is the same as InitPut (with `failOnTombstones=false`) when we don't expect the key to exist, so we replace the latter with the former throughout the code base. If the key might already exist, then we need to use CPut with the possible value and `AllowIfDoesNotExist` set to `true` (which is the case for the backfill code where the value might exist when performing the primary key change). This paves the way to deprecating the InitPut request altogether. Release note: None
It was now only used it tests, so we remove the client side support of InitPut requests altogether. The server side support can be removed once the compatibility with 24.3 is no longer needed. Release note: None
|
This change is |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First commit is #138706.
Addresses: #71074.
Epic: None
kv: remove client side support of FailOnTombstones option of InitPut
We recently removed the only usage of
FailOnTombstonesoption of the InitPut, so we can remove the client side support for it completely now.sql: replace InitPut with CPut
CPut with empty
expValueparameter is the same as InitPut (withfailOnTombstones=false) when we don't expect the key to exist, so we replace the latter with the former throughout the code base. If the key might already exist, then we need to use CPut with the possible value andAllowIfDoesNotExistset totrue(which is the case for the backfill code where the value might exist when performing the primary key change). This paves the way to deprecating the InitPut request altogether.kv: remove client side support of InitPut requests
It was now only used it tests, so we remove the client side support of InitPut requests altogether. The server side support can be removed once the compatibility with 24.3 is no longer needed.
Release note: None