streamingccl: store replicated time in details #103835

stevendanna · 2023-05-24T12:49:10Z

The cutover process uses the progress field to record how many ranges need to be reverted. This, however, wipes out the high watermark that was previously stored in that progress field.

Here, we move the canonical copy of the high watermark to the progress details.

For clarity, we rename "high watermark" as "replicated time".

Further, we delete a long skipped test that isn't providing much value.

Fixes #103534

Epic: none

Release note: None

cockroach-teamcity · 2023-05-24T12:49:22Z

This change is

msbutler

thanks for doing this! I think the only thing I'm unsure about is if we should even continue logging the hwm. I don't think we should as it opens the door to footguns, but curious why you think we should.

pkg/ccl/streamingccl/streamingest/stream_ingestion_job.go

msbutler · 2023-05-24T13:39:58Z

pkg/jobs/jobspb/jobs.proto

@@ -151,6 +151,11 @@ message StreamIngestionProgress {
  // consistent state as of the CutoverTime.
  util.hlc.Timestamp cutover_time = 1 [(gogoproto.nullable) = false];

+  // ReplicatedTime is the ingestion frontier. This is the canonical
+  // value of the frontier. The HighWater in the job progress is for
+  // informational purposes only.


I feel like we should never log the HighWater in the job progress?

pkg/ccl/streamingccl/streamingest/stream_ingestion_job.go

pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go

msbutler · 2023-05-24T13:58:21Z

pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go

+		// Keep the recorded replicatedTime empty until some advancement has been made
+		if sf.replicatedTimeAtStart.Less(replicatedTime) {
+			streamProgress.ReplicatedTime = replicatedTime
+			// The HighWater is for informational purposes


I'm not sure we even want to record the hwm, as it opens the door to foot guns.

pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor_test.go

pkg/ccl/streamingccl/streamingest/replication_stream_e2e_test.go

msbutler · 2023-05-24T17:01:20Z

pkg/ccl/streamingccl/streamingest/stream_ingestion_job_test.go

+		streamProgress := progress.Details.(*jobspb.Progress_StreamIngest).StreamIngest
+		streamProgress.ReplicatedTime = hlcReplicatedTime
+		progress.Progress = &jobspb.Progress_HighWater{
+			HighWater: &hlcReplicatedTime,


as I've said elsewhere, I wonder if we should not record the HighWater in Progress.

msbutler · 2023-05-24T17:02:05Z

pkg/ccl/streamingccl/streamingest/stream_ingestion_job_test.go

+		streamProgress := progress.Details.(*jobspb.Progress_StreamIngest).StreamIngest
+		streamProgress.ReplicatedTime = cutover
+		progress.Progress = &jobspb.Progress_HighWater{
+			HighWater: &cutover,


nit: highWater log alert.

pkg/ccl/streamingccl/streamingest/stream_ingestion_processor.go

msbutler

thanks for doing this! I think the only thing I'm unsure about is if we should even continue logging the hwm. I don't think we should as it opens the door to footguns, but curious why you think we should.

stevendanna · 2023-05-24T17:22:08Z

@msbutler My only hesitation about removing the high watermark completely is that it allows it to be visible in DBConsole, but I agree it is a bit of a footgun.

msbutler · 2023-05-24T17:31:03Z

it allows it to be visible in DBConsole

Ah I see. FWIW, I hadn't even realized the dbconsole shows the hwm. I feel its fine to drop it from the the db console. It's a time series metric anyway, and I think the replication lag graph provides richer information.

one other thing: I think there's some roachtest code you'll need to update as well.

The cutover process uses the progress field to record how many ranges need to be reverted. This, however, wipes out the high watermark that was previously stored in that progress field. Here, we move the canonical copy of the high watermark to the progress details. For clarity, we rename "high watermark" as "replicated time". Further, we delete a long skipped test that isn't providing much value. Epic: none Release note: None

stevendanna · 2023-05-25T10:31:36Z

Updated the roachtests and addressed comments other than the high watermark issue. Going to spend a few minutes looking into the ux differences

msbutler · 2023-05-25T13:00:05Z

hrm, I don't see any updates to pkg/cmd/roachtest/tests/cluster_to_cluster.go ?

I defer to you on the dbconsole question. I'll approve whatever.

msbutler · 2023-05-25T13:02:15Z

one more nit: update the commit/PR message to Fix the original issue? #103534

stevendanna · 2023-05-25T13:09:01Z

hrm, I don't see any updates to pkg/cmd/roachtest/tests/cluster_to_cluster.go ?

Should be here: https://github.com/cockroachdb/cockroach/pull/103835/files#diff-b713f571096d10de2fe8f435442a825c8b41faaa685a6606a8432a61cf663f02

pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go

msbutler · 2023-05-25T13:14:53Z

pkg/cmd/roachtest/tests/cluster_to_cluster.go

-func (c *streamIngesitonJobInfo) GetHighWater() time.Time    { return c.highwaterTime }
+// GetHighWater returns the replicated time. The GetHighWater name is
+// retained here as this is implementing the jobInfo interface used by
+// the latency verifier.


stevendanna · 2023-05-25T13:23:35Z

bors r=msbutler

craig · 2023-05-25T13:51:30Z

Build failed:

Bazel Essential CI (Cockroach)

msbutler · 2023-05-25T16:05:31Z

unrelated flake, logged issue here.

msbutler · 2023-05-25T16:05:37Z

bors retry

craig · 2023-05-25T17:06:55Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-05-25T17:07:02Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 327947f to blathers/backport-release-23.1-103835: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

stevendanna requested review from a team as code owners May 24, 2023 12:49

stevendanna requested review from lidorcarmel and msirek and removed request for a team May 24, 2023 12:49

stevendanna added the backport-23.1.x Flags PRs that need to be backported to 23.1 label May 24, 2023

msbutler reviewed May 24, 2023

View reviewed changes

stevendanna force-pushed the replicated-time-refactor branch from 0b2499a to 327947f Compare May 25, 2023 10:22

stevendanna requested a review from a team as a code owner May 25, 2023 10:22

stevendanna requested review from smg260 and renatolabs and removed request for a team May 25, 2023 10:22

msbutler mentioned this pull request May 25, 2023

c2c: dont swallow pts update error #103834

Merged

msbutler approved these changes May 25, 2023

View reviewed changes

craig bot merged commit 0ab47ef into cockroachdb:master May 25, 2023

stevendanna mentioned this pull request May 26, 2023

release-23.1: streamingccl: store replicated time in details #103941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

streamingccl: store replicated time in details #103835

streamingccl: store replicated time in details #103835

stevendanna commented May 24, 2023 •

edited

Loading

cockroach-teamcity commented May 24, 2023

msbutler left a comment

msbutler May 24, 2023

msbutler May 24, 2023

msbutler May 24, 2023

msbutler May 24, 2023

msbutler left a comment

stevendanna commented May 24, 2023

msbutler commented May 24, 2023

stevendanna commented May 25, 2023

msbutler commented May 25, 2023

msbutler commented May 25, 2023

stevendanna commented May 25, 2023

msbutler May 25, 2023

stevendanna commented May 25, 2023

craig bot commented May 25, 2023

msbutler commented May 25, 2023

msbutler commented May 25, 2023

craig bot commented May 25, 2023

blathers-crl bot commented May 25, 2023

streamingccl: store replicated time in details #103835

streamingccl: store replicated time in details #103835

Conversation

stevendanna commented May 24, 2023 • edited Loading

cockroach-teamcity commented May 24, 2023

msbutler left a comment

Choose a reason for hiding this comment

msbutler May 24, 2023

Choose a reason for hiding this comment

msbutler May 24, 2023

Choose a reason for hiding this comment

msbutler May 24, 2023

Choose a reason for hiding this comment

msbutler May 24, 2023

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

stevendanna commented May 24, 2023

msbutler commented May 24, 2023

stevendanna commented May 25, 2023

msbutler commented May 25, 2023

msbutler commented May 25, 2023

stevendanna commented May 25, 2023

msbutler May 25, 2023

Choose a reason for hiding this comment

stevendanna commented May 25, 2023

craig bot commented May 25, 2023

msbutler commented May 25, 2023

msbutler commented May 25, 2023

craig bot commented May 25, 2023

blathers-crl bot commented May 25, 2023

stevendanna commented May 24, 2023 •

edited

Loading