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

backupccl,importccl: use RandomizeLeases for better leaseholder balance #36665

Merged
merged 2 commits into from Apr 9, 2019

Conversation

Projects
None yet
4 participants
@danhhz
Copy link
Contributor

commented Apr 9, 2019

This is a hack but as a holdover it seems pretty effective.

RESTORE without RandomizeLeases
Screen Shot 2019-04-08 at 3 42 00 PM

RESTORE with RandomizeLeases
Screen Shot 2019-04-08 at 3 47 33 PM

IMPORT without RandomizeLeases
Screen Shot 2019-04-08 at 4 14 09 PM

IMPORT with RandomizeLeases
Screen Shot 2019-04-08 at 4 30 04 PM

danhhz added some commits Apr 9, 2019

backupccl: use RandomizeLeases in RESTORE for better leaseholder balance
This is a bit of a hack, but it seems to be an effective one (see the PR
that added it for graphs). As of the commit that added this, scatter is
not very good at actually balancing leases. This is likely for two
reasons: 1) there's almost certainly some regression in scatter's
behavior, it used to work much better and 2) scatter has to operate by
balancing leases for all ranges in a cluster, but in RESTORE, we really
just want it to be balancing the span being restored into.

Release note (bug fix): RESTORE now balances the work of ingesting data
more evenly between nodes.
importccl: use RandomizeLeases in IMPORT for better leaseholder balance
This is a bit of a hack, but it seems to be an effective one (see the PR
that added it for graphs). As of the commit that added this, scatter is
not very good at actually balancing leases. This is likely for two
reasons: 1) there's almost certainly some regression in scatter's
behavior, it used to work much better and 2) scatter has to operate by
balancing leases for all ranges in a cluster, but in IMPORT, we really
just want it to be balancing the span being imported into.

Release note (bug fix): IMPORT now balances the work of ingesting data
more evenly between nodes.

@danhhz danhhz requested review from dt and tbg Apr 9, 2019

@danhhz danhhz requested a review from cockroachdb/sql-bulk-prs as a code owner Apr 9, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

This change is Reviewable

@dt

dt approved these changes Apr 9, 2019

@tbg

tbg approved these changes Apr 9, 2019

@danhhz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Flake is #36666. Thanks for the reviews!

bors r=dt,tbg

craig bot pushed a commit that referenced this pull request Apr 9, 2019

Merge #36665
36665: backupccl,importccl: use RandomizeLeases for better leaseholder balance r=dt,tbg a=danhhz

This is a hack but as a holdover it seems pretty effective.

RESTORE without RandomizeLeases
<img width="823" alt="Screen Shot 2019-04-08 at 3 42 00 PM" src="https://user-images.githubusercontent.com/52528/55810382-517c2800-5a9c-11e9-9322-705bc825cde8.png">

RESTORE with RandomizeLeases
<img width="867" alt="Screen Shot 2019-04-08 at 3 47 33 PM" src="https://user-images.githubusercontent.com/52528/55810383-517c2800-5a9c-11e9-8763-5bf40f1fd09b.png">

IMPORT without RandomizeLeases
<img width="881" alt="Screen Shot 2019-04-08 at 4 14 09 PM" src="https://user-images.githubusercontent.com/52528/55810384-517c2800-5a9c-11e9-9075-0951d347ffe2.png">

IMPORT with RandomizeLeases
<img width="911" alt="Screen Shot 2019-04-08 at 4 30 04 PM" src="https://user-images.githubusercontent.com/52528/55810385-517c2800-5a9c-11e9-9e89-02fcc7fadad1.png">


Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
@craig

This comment has been minimized.

Copy link

commented Apr 9, 2019

Build failed

@tbg

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

bors r=dt,tbg

I just queued the skip for that test, so this time it should fail because of something else 👿

craig bot pushed a commit that referenced this pull request Apr 9, 2019

Merge #36629 #36639 #36665
36629: opt: clean up buildAggregation in optbuilder r=rytaft a=rytaft

`buildAggregation` in `opt/optbuilder/groupby.go` was a monster of a function
with a lot of parameters. This commit splits it into a few logical pieces
to improve maintainability.

Closes #29215

Release note: None

36639: opt: Fold functions that are contained in a whitelist r=rytaft a=rytaft

This commit adds a whitelist of functions that are always ok
to fold during normalization, because they always produce the same
results given the same set of inputs. This will allow us to generate
spans for predicates that contain constant functions.

This is not the ideal way to solve this problem, but it provides
a stopgap until #26582 is addressed.

Fixes #31617

Release note (performance improvement): Improved the performance of
some queries containing predicates with constant functions, since
these functions are now evaluated earlier during query optimization.

36665: backupccl,importccl: use RandomizeLeases for better leaseholder balance r=dt,tbg a=danhhz

This is a hack but as a holdover it seems pretty effective.

RESTORE without RandomizeLeases
<img width="823" alt="Screen Shot 2019-04-08 at 3 42 00 PM" src="https://user-images.githubusercontent.com/52528/55810382-517c2800-5a9c-11e9-9322-705bc825cde8.png">

RESTORE with RandomizeLeases
<img width="867" alt="Screen Shot 2019-04-08 at 3 47 33 PM" src="https://user-images.githubusercontent.com/52528/55810383-517c2800-5a9c-11e9-8763-5bf40f1fd09b.png">

IMPORT without RandomizeLeases
<img width="881" alt="Screen Shot 2019-04-08 at 4 14 09 PM" src="https://user-images.githubusercontent.com/52528/55810384-517c2800-5a9c-11e9-9075-0951d347ffe2.png">

IMPORT with RandomizeLeases
<img width="911" alt="Screen Shot 2019-04-08 at 4 30 04 PM" src="https://user-images.githubusercontent.com/52528/55810385-517c2800-5a9c-11e9-9e89-02fcc7fadad1.png">


Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
@craig

This comment has been minimized.

Copy link

commented Apr 9, 2019

Build succeeded

@craig craig bot merged commit 1cdb53e into cockroachdb:master Apr 9, 2019

2 of 3 checks passed

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

@danhhz danhhz deleted the danhhz:scatter branch Apr 9, 2019

@danhhz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@dt you mentioned offline that we might want to backport at least the restore commit to 19.1. I'd like to let it bake for a while, so not targeting 19.1.0. Maybe 19.1.1 or 19.1.2? We may even have a better fix by then, this still seems to me like a workaround for Scatter without RandomizeLeases not working in a satisfying way

@danhhz

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Okay, I'm unlikely to get to a better fix in the immediate future and this doesn't seem to have caused any issues on master. @dt @tbg thoughts on backporting this into 19.1.2?

@tbg

This comment has been minimized.

Copy link
Member

commented May 15, 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.