changefeedccl: uri sinks, fill in record key, WITH envelope by danhhz · Pull Request #25542 · cockroachdb/cockroach

danhhz · 2018-05-16T04:01:13Z

See commit messages for details

cockroach-teamcity · 2018-05-16T04:01:22Z

This change is

tbg · 2018-05-17T10:00:46Z

mod nits

Reviewed 8 of 8 files at r1, 2 of 2 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

pkg/ccl/changefeedccl/changefeed.go, line 224 at r3 (raw file):

		colIdxMap := make(map[sqlbase.ColumnID]int)
		var valNeededForCol util.FastIntSet
		if cf.envelope == optEnvelopeKeyOnly {

nit:

var colIDs []sometype
if cf.envelope == optEnvelopeKeyOnly {
  colIDs = tableDesc.PrimaryIndex.ColumnIDs
} else {
  colIDs = tableDesc.Columns
}
for ...

pkg/ccl/changefeedccl/changefeed.go, line 365 at r3 (raw file):

		jsonKey.Format(&key)
		var value bytes.Buffer
		if cf.envelope == optEnvelopeNone {

I'm not quite sure what the "envelope" stands for, but is it intuitive that "none" corresponds to "both"? Should it just be optEnvelopeBoth?

pkg/ccl/changefeedccl/changefeed_test.go, line 80 at r3 (raw file):

	const testPollingInterval = 10 * time.Millisecond
	defer func(oldInterval time.Duration) {
		jobs.DefaultAdoptInterval = oldInterval

Since this is new code, can we do the Right Thing™ and pass this as a testing knob instead of futzing with a global?

pkg/ccl/changefeedccl/changefeed_test.go, line 97 at r3 (raw file):

testProducersHook

Another candidate to move into a knob.

Comments from Reviewable

danhhz · 2018-05-17T16:55:58Z

Review status: 6 of 8 files reviewed at latest revision, 4 unresolved discussions.

pkg/ccl/changefeedccl/changefeed.go, line 224 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

nit:

var colIDs []sometype
if cf.envelope == optEnvelopeKeyOnly {
  colIDs = tableDesc.PrimaryIndex.ColumnIDs
} else {
  colIDs = tableDesc.Columns
}
for ...

Done

pkg/ccl/changefeedccl/changefeed.go, line 365 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

I'm not quite sure what the "envelope" stands for, but is it intuitive that "none" corresponds to "both"? Should it just be optEnvelopeBoth?

yeah. "envelope" make more sense when the third value gets introduced, which is both the new and old row values. how about optEnvelopeRow for this?

pkg/ccl/changefeedccl/changefeed_test.go, line 80 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Since this is new code, can we do the Right Thing™ and pass this as a testing knob instead of futzing with a global?

Sure, though I'd like to do it on another PR if that's alright. There's a bunch of other references to this that should also get updated. #25622

pkg/ccl/changefeedccl/changefeed_test.go, line 97 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

testProducersHook
Another candidate to move into a knob.

I think the plan is to replace this with the sql table sink that ben suggested in the rfc

Comments from Reviewable

tbg · 2018-05-17T17:09:14Z

Still LGTM.

Reviewed 2 of 2 files at r4, 2 of 2 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

pkg/ccl/changefeedccl/changefeed.go, line 365 at r3 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

yeah. "envelope" make more sense when the third value gets introduced, which is both the new and old row values. how about optEnvelopeRow for this?

I'm not really in the know here, but optEnvelopeRow SGTM.

pkg/ccl/changefeedccl/changefeed_test.go, line 80 at r3 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

Sure, though I'd like to do it on another PR if that's alright. There's a bunch of other references to this that should also get updated. #25622

Sure, thanks!

pkg/ccl/changefeedccl/changefeed_test.go, line 97 at r3 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

I think the plan is to replace this with the sql table sink that ben suggested in the rfc

👍 that's even better

Comments from Reviewable

danhhz · 2018-05-17T17:22:12Z

TFTR!

bors r+

craig · 2018-05-17T17:36:11Z

Build failed

GitHub CI (Cockroach)

danhhz · 2018-05-17T17:54:00Z

bors r=tschottdorf

tbg · 2018-05-17T18:14:34Z

Could you triage the build failure? See #25616 (comment)

danhhz · 2018-05-17T18:23:05Z

I looked at it before rerunning, but the test timed out and failed most of the logic tests. Are we really posting that for every flake now? I haven't heard it announced anywhere

craig · 2018-05-17T18:27:20Z

Build failed (retrying...)

GitHub CI (Cockroach)

danhhz · 2018-05-17T18:32:24Z

flake in TestReplicaLeaseRejectUnknownRaftNodeID

replica_test.go:1206: unexpected error obtaining lease for invalid store: cannot replace lease repl=(n1,s1):1 seq=1 start=0.000000000,0 exp=1.800000124,3 pro=0.900000124,4 with repl=(n2,s2):2 seq=0 start=0.900000124,0 exp=0.900000134,0: requested lease overlaps previous lease

craig · 2018-05-17T19:06:56Z

Merge conflict (retrying...)

Currently `kafka://` is the only supported scheme. The bootstrap servers are specified via the uri host:port. The kafka `topic_prefix` is now specified via uri param. Release note: None

Release note: None

`none` is the default and emits the primary key as the record key and the full updated row as the record value. `key_only` emits the same key but leaves the value empty. Release note: None

nvb · 2018-05-17T20:08:49Z

Looks like the flake is #25568. I'll help @a-robinson investigate.

nvb · 2018-05-17T20:11:09Z

Never mind, he already has a fix out #25625.

danhhz · 2018-05-17T20:20:44Z

okay lets try this again. not sure why bors thought it had a merge conflict

bors r=tschottdorf

tbg · 2018-05-17T20:37:56Z

I looked at it before rerunning, but the test timed out and failed most of the logic tests. Are we really posting that for every flake now? I haven't heard it announced anywhere

This is just a favor I ask, since it helps me keep track of what's urgent. Most of our builds are on PR branches, not master. Also, the authors already look at the failed build anyway (or how could they be sure that they didn't introduce the problem) so it's just a small step to let the rest of the world know what went wrong as well.

Ideally, of course, the bors message would contain the snippet from bors.. filed #25632.

25542: changefeedccl: uri sinks, fill in record key, WITH envelope r=tschottdorf a=danhhz See commit messages for details 25631: storage: ensure non-expired context before each liveness update attempt r=nvanbenschoten a=nvanbenschoten Fixes #25430. Before this change, a liveness update could get stuck in an infinite loop if its context expired. This is because it would continue to retry and continue to get `errRetryLiveness` errors due to `AmbiguousResultErrors` created by `DistSender`. Release note: None Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>

craig · 2018-05-17T20:55:22Z

Build succeeded

GitHub CI (Cockroach)

danhhz requested review from a team, nvb and tbg May 16, 2018 04:01

danhhz force-pushed the cdc_key branch 2 times, most recently from 626c6e7 to c29bfac Compare May 16, 2018 19:29

danhhz mentioned this pull request May 17, 2018

jobs: replace DefaultAdoptInterval with a testing knob #25622

Closed

danhhz force-pushed the cdc_key branch from c29bfac to a7ea343 Compare May 17, 2018 16:55

danhhz added 3 commits May 17, 2018 15:47

changefeedccl: switch to a uri-based sink configuration

257211f

Currently `kafka://` is the only supported scheme. The bootstrap servers are specified via the uri host:port. The kafka `topic_prefix` is now specified via uri param. Release note: None

changefeedccl: emit the row's primary key in changefeed records

a90b835

Release note: None

changefeedccl: support WITH envelope={none,key_only}

142c964

`none` is the default and emits the primary key as the record key and the full updated row as the record value. `key_only` emits the same key but leaves the value empty. Release note: None

danhhz force-pushed the cdc_key branch from a7ea343 to 142c964 Compare May 17, 2018 19:47

craig bot merged commit 142c964 into cockroachdb:master May 17, 2018

danhhz deleted the cdc_key branch June 12, 2018 15:45

Conversation

danhhz commented May 16, 2018

Uh oh!

cockroach-teamcity commented May 16, 2018

Uh oh!

tbg commented May 17, 2018

Uh oh!

danhhz commented May 17, 2018

Uh oh!

tbg commented May 17, 2018

Uh oh!

danhhz commented May 17, 2018

Uh oh!

craig bot commented May 17, 2018

Build failed

Uh oh!

danhhz commented May 17, 2018

Uh oh!

tbg commented May 17, 2018

Uh oh!

danhhz commented May 17, 2018

Uh oh!

craig bot commented May 17, 2018

Build failed (retrying...)

Uh oh!

danhhz commented May 17, 2018

Uh oh!

craig bot commented May 17, 2018

Merge conflict (retrying...)

Uh oh!

nvb commented May 17, 2018

Uh oh!

nvb commented May 17, 2018

Uh oh!

danhhz commented May 17, 2018

Uh oh!

tbg commented May 17, 2018

Uh oh!

craig bot commented May 17, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants