hlc: remove the Synthetic field from Timestamp and LegacyTimestamp#116830
hlc: remove the Synthetic field from Timestamp and LegacyTimestamp#116830craig[bot] merged 6 commits intocockroachdb:masterfrom
Conversation
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
0f10147 to
b989349
Compare
|
|
61a81fe to
8a70b72
Compare
e2190ca to
c70a9d4
Compare
117262: storage: don't set synthetic timestamps in tests r=nvanbenschoten a=nvanbenschoten Informs #101938. Extracted from #116830. We are about to remove this field from the proto, so stop assigning it in storage tests. `TestMVCCAndEngineKeyDecodeSyntheticTimestamp` still tests that we can properly decode keys that were encoded with synthetic timestamps. Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
2aac63d to
af0eef6
Compare
af0eef6 to
c31785b
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, 9 of 9 files at r4, 2 of 2 files at r5, 1 of 1 files at r6, 15 of 15 files at r7, 8 of 8 files at r8, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @miretskiy, @nvanbenschoten, and @RaduBerinde)
pkg/sql/sem/asof/as_of.go line 249 at r6 (raw file):
// Parse synthetic flag. syn := false if strings.HasSuffix(s, "?") {
This is strictly a breaking SQL change. Do we need a release note for it?
pkg/ccl/changefeedccl/helpers_test.go line 635 at r4 (raw file):
} if !options.disableSyntheticTimestamps && rand.Intn(2) == 0 { // Offset all timestamps a random (but consistent per test) amount into the
Do we want to keep this test coverage of future timestamps around? I see that the actual offsetting is commented out anyway, so it's not currently in effect, but seems like it might be useful to fix that at some point and keep the deactivated code around.
@miretskiy Do you have an opinion on this? If not, let's axe it.
Informs cockroachdb#101938. This commit is essentially a revert of 951add4. It removes the handling of synthetic timestamps from the ptstorage package. This flag has been deprecated since v22.2 and is no longer consulted in uncertainty interval checks or by transaction commit-wait. It will never makes its way into the ptstorage package once it is removed from the proto definition. Release note: None
Informs cockroachdb#101938. This commit is essentially a revert of 5259c1d. It removes the handling of synthetic timestamps from the changefeedccl package. This flag has been deprecated since v22.2 and is no longer consulted in uncertainty interval checks or by transaction commit-wait. It will never makes its way into the changefeedccl package once it is removed from the proto definition. Release note: None
Informs cockroachdb#101938. This is now unused. Release note: None
Informs cockroachdb#101938. This commit is essentially a revert of 34dc5ae. It removes the ability to pass a synthetic timestamp to an AOST query. Release note (backward-incompatible change): AS OF SYSTEM TIME queries can no longer use a timestamp followed by a question mark to signifiy a future-time value. This was an undocumented syntax.
Closes cockroachdb#101938. This commit completes the work to remove the Synthetic field from Timestamp and LegacyTimestamp. It removes the fields from the proto definitions and removes all access to the fields in methods. The commit also cleans up the remaining references in the code which were just waiting for the field to be removed. Release note: None
Informs cockroachdb#101938. Release note: None
c31785b to
6f48942
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTR!
bors r=erikgrinaker
Reviewable status:
pkg/sql/sem/asof/as_of.go line 249 at r6 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
This is strictly a breaking SQL change. Do we need a release note for it?
Good call. Done.
pkg/ccl/changefeedccl/helpers_test.go line 635 at r4 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Do we want to keep this test coverage of future timestamps around? I see that the actual offsetting is commented out anyway, so it's not currently in effect, but seems like it might be useful to fix that at some point and keep the deactivated code around.
@miretskiy Do you have an opinion on this? If not, let's axe it.
I'll remove for now, as this is dead code without synthetic timestamps and removing it allows us to remove a decent amount of test infrastructure. I'll reintroduce it if @miretskiy thinks it's valuable.
|
Build succeeded: |
Addresses a TODO. This method has been unnecessary since synthetic timestamps were removed in cockroachdb#116830. Release note: None
Closes #101938.
This PR completes the work to remove the
Syntheticfield fromTimestampandLegacyTimestamp. It removes the remaining uses, removes the fields from the proto definitions, and removes all access to the fields in methods.Release note: None