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

distsql: remove legacy batched-write index dist-backfiller #37213

Merged
merged 1 commit into from May 7, 2019

Conversation

Projects
None yet
4 participants
@dt
Copy link
Member

commented Apr 30, 2019

This removes the legacy batched-writes implementaiton for the distributed index backfiller.

This version was off-by-default in 19.1 in favor of the SST-ingestion implementation, but was still available via
a setting in case anyone encountered problems with the new backfiller. However maintaining both long term is
undesirable and the SST-based ingestion implementation will have had two development cycles (19.1 development and
19.2 polish) when 19.2 is released, so keeping the slower legacy implementation around seems less worth it.

Release note (sql change): removed the legacy batch-by-batch index backfiller, as well as the 'schemachanger.bulk_index_backfill.enabled' setting that could be disabled to activate it

@dt dt requested review from lucy-zhang and vivekmenezes Apr 30, 2019

@dt dt requested review from cockroachdb/distsql-prs as code owners Apr 30, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

This change is Reviewable

@lucy-zhang
Copy link
Contributor

left a comment

Does the cluster setting need to be deprecated or anything?

distsql: remove legacy batched-write index dist-backfiller
This removes the legacy batched-writes implementaiton for the distributed index backfiller.

This version was off-by-default in 19.1 in favor of the SST-ingestion implementation, but was still available via
a setting in case anyone encountered problems with the new backfiller. However maintaining both long term is
undesirable and the SST-based ingestion implementation will have had two development cycles (19.1 development and
19.2 polish) when 19.2 is released, so keeping the slower legacy implementation around seems less worth it.

Release note (sql change): removed the legacy batch-by-batch index backfiller, as well as the 'schemachanger.bulk_index_backfill.enabled' setting that could be disabled to activate it

@dt dt force-pushed the dt:delete-old-backfiller branch from 94c7f1b to ca963c8 May 7, 2019

@dt dt requested a review from cockroachdb/sql-async-prs as a code owner May 7, 2019

@@ -43,6 +43,8 @@ var retiredSettings = map[string]struct{}{
"kv.allocator.stat_rebalance_threshold": {},
// removed as of 2.2.
"kv.raft_log.synchronize": {},
// removed as of 19.2.
"schemachanger.bulk_index_backfill.enabled": {},

This comment has been minimized.

Copy link
@dt

dt May 7, 2019

Author Member

@lucy-zhang this list is the deprecated settings names

@dt

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

bors r+

@dt dt closed this May 7, 2019

@craig

This comment has been minimized.

Copy link

commented May 7, 2019

Not awaiting review

@dt dt reopened this May 7, 2019

@dt

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

bors r+

but let's try the other button this time

craig bot pushed a commit that referenced this pull request May 7, 2019

Merge #37213
37213: distsql: remove legacy batched-write index dist-backfiller r=dt a=dt

This removes the legacy batched-writes implementaiton for the distributed index backfiller.

This version was off-by-default in 19.1 in favor of the SST-ingestion implementation, but was still available via
a setting in case anyone encountered problems with the new backfiller. However maintaining both long term is
undesirable and the SST-based ingestion implementation will have had two development cycles (19.1 development and
19.2 polish) when 19.2 is released, so keeping the slower legacy implementation around seems less worth it.

Release note (sql change): removed the legacy batch-by-batch index backfiller, as well as the 'schemachanger.bulk_index_backfill.enabled' setting that could be disabled to activate it

Co-authored-by: David Taylor <tinystatemachine@gmail.com>
@craig

This comment has been minimized.

Copy link

commented May 7, 2019

Build succeeded

@craig craig bot merged commit ca963c8 into cockroachdb:master May 7, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@dt dt deleted the dt:delete-old-backfiller branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.