streamingest: c2c UX fixes #93755
Merged
streamingest: c2c UX fixes #93755
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 change is |
lidorcarmel
force-pushed
the
lidor_c2c_ux_fixes
branch
4 times, most recently
from
9fc0134 to
45a2ea9
Compare
stevendanna
reviewed
Dec 19, 2022
| var alterReplicationJobHeader = colinfo.ResultColumns{ | ||
| {Name: "replication_job_id", Typ: types.Int}, | ||
| var alterReplicationCutoverHeader = colinfo.ResultColumns{ | ||
| {Name: "cutover_time", Typ: types.TimestampTZ}, |
There was a problem hiding this comment.
I think we lose precision here if we had a logical component to the cutover timestamp.
There was a problem hiding this comment.
interesting, thanks. I did not think about that.. we're also losing nanos when cutting over to a time from SHOW. I might nag you tomorrow about this if I don't have a better way.
There was a problem hiding this comment.
I guess we have at least a couple of options:
- print a not-so-pretty hlc such as this:
[email protected]:26257/defaultdb> SELECT cluster_logical_timestamp();
cluster_logical_timestamp
----------------------------------
1671493837007535000.0000000000
(1 row)
- print a less accurate timestamp (current pr) without logical and without nanos:
[email protected]:26257/defaultdb> alter tenant dest5 complete replication to latest;
cutover_time
---------------------------------
2022-12-06 00:25:02.098763+00
(1 row)
similar to:
[email protected]:26257/defaultdb> SELECT hlc_to_timestamp(cluster_logical_timestamp());
hlc_to_timestamp
---------------------------------
2022-12-19 23:50:55.100638+00
(1 row)
I was thinking that we need something that users can understand and not hlc.
in which scenarios are we going to have issues because of accuracy?
There was a problem hiding this comment.
in which scenarios are we going to have issues because of accuracy?
I think the main use of this time in a context where the accuracy may matter is if you are using it to do comparisons using AOST queries or crdb_internal.fingeprint or similar. In that context, the loss of accuracy is likely OK provided that the timestamp printed is inside the GC window on both sides and if we are inaccurate in the correct direction.
I do like having it readable easily by users without having to convert it.
There was a problem hiding this comment.
updated, PTAL.
pretty time for replication and retained times, hlc for cutover.
lidorcarmel
force-pushed
the
lidor_c2c_ux_fixes
branch
2 times, most recently
from
e507769 to
70edfac
Compare
lidorcarmel
requested review from
a team,
herkolategan,
smg260 and
adityamaru
and removed request for
a team
lidorcarmel
force-pushed
the
lidor_c2c_ux_fixes
branch
4 times, most recently
from
27b6397 to
120a1ae
Compare
Currently the retained time (min, or oldest we can cutover to) and the replicated time (max replicated time) are converted from an accurate HLC to a human readable timestamp, rounded to microseconds. Rounding means we might get the min or max time outside the time window, where the user cannot cutover to that timestamp. Instead or rounding this PR truncates the max timestamp and prints the ceil of the min timestamp, so that both of those timestamps are legal cutover timestmaps. Epic: CRDB-18752 Release note: None
And simplify the time conversion to datum a bit. Epic: CRDB-18752 Release note: None
Currently we print the producer and ingestion job ids. Instead, users should not see job ids unless something is really broken. Epic: CRDB-18752 Release note: None
ALTER TENANT PAUSE/RESUME: currently prints the job id, and this PR removes the output. ALTER TENANT COMPLETE: currently prints the job id. This PR changes the output to be the cutover timestamp. The rational is that the user may not know the cutover time because LATEST or NOW() etc. was used. Epic: CRDB-18752 Release note: None
lidorcarmel
force-pushed
the
lidor_c2c_ux_fixes
branch
from
120a1ae to
01d22ea
Compare
stevendanna
approved these changes
Jan 6, 2023
|
TFTR!! |
|
Build succeeded: |
msbutler
added a commit
to msbutler/cockroach
that referenced
this pull request
Jan 8, 2023
c2c/tpcc failed due to cockroachdb#93755. This patch fixes the roachtest. Fixes cockroachdb#94877 Release note: None Epic: None
kvoli
pushed a commit
to kvoli/cockroach
that referenced
this pull request
Jan 10, 2023
93755: streamingest: c2c UX fixes r=lidorcarmel a=lidorcarmel CREATE TENANT FROM REPLICATION: currently has an output (the job ids of the producer and consumer). This PR removes the output. ALTER TENANT PAUSE/RESUME: currently prints the job id, and this PR removes the output. ALTER TENANT COMPLETE: currently prints the job id. This PR changes the output to be the cutover timestamp. The rational is that the user may not know the cutover time because LATEST or NOW() etc. was used. Fixes: cockroachdb#94706 Epic: CRDB-18752 Release note: None 94695: sql,descs,*: rewrite by-ID and by-name descriptor lookups r=postamar a=postamar This commit replaces all Get(Mutable|Immutable)*By(ID|Name) methods on the descs.Collection and their usages with the recently-introduced By(ID|Name)Getter methods. While this commit shouldn't fundamentally change anything as far as functionality is concerned, it isn't strictly-speaking a refactor because the default behaviors have been changed slightly to make them consistent accross similar kinds of lookups. Here's an illustrative example: previously, GetImmutableDatabaseByID and GetImmutableSchemaByID would honor the Required flag while the other GetImmutable*ByID methods would ignore it and return an error when the descriptor is not found, presently all immutable by-ID lookups always return an error when the descriptor is not found. The new API does away with the tree.(Common|Object)LookupFlags and these have been cleaned up because now only the resolver uses them. Fixes cockroachdb#87753. Release note: None Co-authored-by: Lidor Carmel <[email protected]> Co-authored-by: Marius Posta <[email protected]>
Closed
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.
CREATE TENANT FROM REPLICATION: currently has an output (the job ids of the producer and consumer). This PR removes the output.
ALTER TENANT PAUSE/RESUME: currently prints the job id, and this PR removes the output.
ALTER TENANT COMPLETE: currently prints the job id. This PR changes the output to be the cutover timestamp. The rational is that the user may not know the cutover time because LATEST or NOW() etc. was used.
Fixes: #94706
Epic: CRDB-18752
Release note: None