Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changefeedccl: ux fixes and tighter tracked spans #29825

Merged
merged 5 commits into from
Sep 10, 2018

Conversation

danhhz
Copy link
Contributor

@danhhz danhhz commented Sep 7, 2018

see commits for details

I thought the SpanResolverIterator was doing span trimming, etc, but it
turns out this lives in level up in `PartitionSpans`. Reuse it.

This was causing performance issues in tests when we grabbed a pre-split
range and didn't trim it, so too much of the keyspace was being watched.
Very occasionally, it would also cause flakes if we ended up watching
part of the timeseries data (which ExportRequest doesn't support).

All `sql` package changes generated mechanically with:

    $ gorename -from '"./pkg/sql".DistSQLPlanner.partitionSpans' -to PartitionSpans
    $ gorename -from '"./pkg/sql".spanPartition' -to SpanPartition
    $ gorename -from '"./pkg/sql".SpanPartition.spans' -to Spans
    $ gorename -from '"./pkg/sql".SpanPartition.node' -to Node

Plus a couple updates to the new name in comments, found via git grep.

Release note (bug fix): avoids a very rare premature failure in
CHANGEFEEDs caused by a race condition with range splits
These came from development partner feedback.

The following options metioned in the rfc now throw a specific "<option>
not yet implemented" error: `format=avro`, `envelope=diff`,
`confluent_schema_registry`, `schema_topic`.

Unrecognized sink parameters now throw an error instead of being
silently ignored.

Release note: None
It snuck in.

Release note: None
@danhhz danhhz requested review from mrtracy and a team September 7, 2018 22:25
@danhhz danhhz requested a review from a team as a code owner September 7, 2018 22:25
@danhhz danhhz requested review from a team September 7, 2018 22:25
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Adding stuff to the buffer can take longer than it did to fetch if the
sink is being slow. Definitely worth timing and logging on the nightly
roachtests.

Release note: None
There's a bit of variation in how long each poll takes (tends to be
between 1m and 4m). I'd like to see what happens if we run the test
longer.

Also sneak in a couple fixes while I'm here. Increase vlog to catch
resolved timestamp updates. Make starting kafka more idempotent (for dev
iteration). Make kafka installation quieter.

Release note: None
Copy link
Contributor

@mrtracy mrtracy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all :LGTM:

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@danhhz
Copy link
Contributor Author

danhhz commented Sep 10, 2018

Thanks for the review!

bors r=mrtracy

craig bot pushed a commit that referenced this pull request Sep 10, 2018
29818: sql: array_to_json builtin r=asubiotto,jordanlewis a=changangela

Relates to #20120.

29825: changefeedccl: ux fixes and tighter tracked spans r=mrtracy a=danhhz

see commits for details

30004: rangefeed: bump TestProcessorSlowConsumer event timeout r=nvanbenschoten a=nvanbenschoten

Fixes #29671.

Before this, the test would fail in about 3 minutes of stress. After this change, it didn't fail in over 30 minutes.

Will need to be backported.

Release note: None

Co-authored-by: changangela <angela@cockroachlabs.com>
Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@craig
Copy link
Contributor

craig bot commented Sep 10, 2018

Build succeeded

@craig craig bot merged commit 4d4e367 into cockroachdb:master Sep 10, 2018
@danhhz danhhz deleted the cdc_fixes branch September 10, 2018 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants