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

Coordinator balancer move then drop fix #5528

Merged
merged 4 commits into from Mar 29, 2018

Conversation

@clintropolis
Copy link
Member

commented Mar 23, 2018

Fixes issue where coordinator balancer will move a segment to the destination server but never actually drop the segment from the source server, one of the issues described in #5521.

With this bug in place, the effective behavior of segment movement was that the balancer would copy the segment to where it thought it needed to be, and the load rule would then decide from where to drop. Because the drop here was never previously working, this may be worth discussion of any implications.

@jihoonson

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@clintropolis thanks for working on this. This PR contains some changes to use lambda and I think it makes me difficult to figure out which parts are bug fixes. I would say it's better to split this PR into two sub PRs for bug fix only and remaining.

@jon-wei jon-wei added the Bug label Mar 27, 2018

@clintropolis

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2018

Sorry for mixing stuff, this was already part of a split up chain of commits that originally also included the stuff in #5529. Main bug fix is here and additions to test to test the original bug are some additions here and the test itself

I can split this stuff out if you prefer, I was just lazily updating stuff as I came across it.

@jihoonson
Copy link
Contributor

left a comment

Thanks @clintropolis. Left some comments.

sourceLoadQueueChildrenCache.start();
destinationLoadQueueChildrenCache.start();

Thread.sleep(100);

This comment has been minimized.

Copy link
@jihoonson

jihoonson Mar 27, 2018

Contributor

This might cause test failures in Travis. If this is to wait for childrenCaches to be ready, some kind of real check (by polling or using a latch) should be used instead. Same for other sleeps.

tearDownServerAndCurator();
}

@Test

This comment has been minimized.

Copy link
@jihoonson

jihoonson Mar 27, 2018

Contributor

Please add a timeout to avoid too long waits even when this test becomes broken in the future.

loadManagementPeons.put("localhost:1", loadQueuePeon);
loadManagementPeons.put("localhost:2", loadQueuePeon2);


This comment has been minimized.

Copy link
@jihoonson

jihoonson Mar 27, 2018

Contributor

Unnecessary line. Please remove other unnecessary lines too.

@jihoonson jihoonson merged commit 30fc4d3 into apache:master Mar 29, 2018

2 checks passed

Inspections: pull requests (Druid) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@clintropolis clintropolis deleted the clintropolis:coordinator-move-drop-fix branch Mar 30, 2018

jon-wei added a commit to implydata/druid that referenced this pull request Apr 4, 2018
Coordinator balancer move then drop fix (apache#5528)
* apache#5521 part 1

* formatting

* oops

* less magic tests
gianm added a commit to implydata/druid that referenced this pull request May 16, 2018
Coordinator balancer move then drop fix (apache#5528)
* apache#5521 part 1

* formatting

* oops

* less magic tests

@jihoonson jihoonson added this to the 0.12.2 milestone Jul 5, 2018

jihoonson added a commit to jihoonson/druid that referenced this pull request Jul 5, 2018
Coordinator balancer move then drop fix (apache#5528)
* apache#5521 part 1

* formatting

* oops

* less magic tests
fjy added a commit that referenced this pull request Jul 6, 2018
Coordinator balancer move then drop fix (#5528) (#5946)
* #5521 part 1

* formatting

* oops

* less magic tests
leventov added a commit to metamx/druid that referenced this pull request Jul 20, 2018
Coordinator balancer move then drop fix (apache#5528) (apache#5946)
* apache#5521 part 1

* formatting

* oops

* less magic tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.