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

Fix outgoing NodeID #28779

Merged
merged 5 commits into from Feb 22, 2018

Conversation

Projects
None yet
5 participants
@czjxy881
Copy link
Contributor

commented Feb 22, 2018

fixes #28777

  • Have you signed the contributor license agreement?
    yes
  • Have you followed the contributor guidelines?
    yes
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2018

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2018

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@czjxy881

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

alreay signed

@DaveCTurner

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

Thanks @czjxy881 - could you add a test that asserts that the message is now correct?

add test case
add test case
@czjxy881

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

@DaveCTurner added

@DaveCTurner
Copy link
Contributor

left a comment

Thanks. I pointed out a handful of tiny formatting problems, a compile error, and I have one further request: it's possible that this assertion is never executed if there was no matching decision. I think we should assert that there is an explanation with this message rather than if there is an explanation of the right type then it has this message.

@@ -299,6 +299,12 @@ public void testOutgoingThrottlesAllocation() {
new MoveAllocationCommand("test", 0, "node2", "node4")), true, false);
assertEquals(commandsResult.explanations().explanations().size(), 1);
assertEquals(commandsResult.explanations().explanations().get(0).decisions().type(), Decision.Type.THROTTLE);
for(Decision decision : commandsResult.explanations().explanations().get(0).decisions().getDecisions()){

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 22, 2018

Contributor

Nit: please add spaces after the for and before the {.

@@ -299,6 +299,12 @@ public void testOutgoingThrottlesAllocation() {
new MoveAllocationCommand("test", 0, "node2", "node4")), true, false);
assertEquals(commandsResult.explanations().explanations().size(), 1);
assertEquals(commandsResult.explanations().explanations().get(0).decisions().type(), Decision.Type.THROTTLE);
for(Decision decision : commandsResult.explanations().explanations().get(0).decisions().getDecisions()){
if(decision.label().equals(ThrottlingAllocationDecider.NAME)){

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 22, 2018

Contributor

Nit: please add spaces after the if and before the {.

for(Decision decision : commandsResult.explanations().explanations().get(0).decisions().getDecisions()){
if(decision.label().equals(ThrottlingAllocationDecider.NAME)){
assertEquals("reached the limit of outgoing shard recoveries [1] on the node [node1] which holds the primary, " +
"cluster setting [cluster.routing.allocation.node_concurrent_outgoing_recoveries=1] (can also be set via [cluster.routing.allocation.node_concurrent_recoveries])",decision.getExplanation());

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 22, 2018

Contributor

This line is longer than 120 characters so CI will reject it.

@@ -299,6 +299,12 @@ public void testOutgoingThrottlesAllocation() {
new MoveAllocationCommand("test", 0, "node2", "node4")), true, false);
assertEquals(commandsResult.explanations().explanations().size(), 1);
assertEquals(commandsResult.explanations().explanations().get(0).decisions().type(), Decision.Type.THROTTLE);
for(Decision decision : commandsResult.explanations().explanations().get(0).decisions().getDecisions()){
if(decision.label().equals(ThrottlingAllocationDecider.NAME)){

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 22, 2018

Contributor

ThrottlingAllocationDecider is not imported, so this is a compile error.

for(Decision decision : commandsResult.explanations().explanations().get(0).decisions().getDecisions()){
if(decision.label().equals(ThrottlingAllocationDecider.NAME)){
assertEquals("reached the limit of outgoing shard recoveries [1] on the node [node1] which holds the primary, " +
"cluster setting [cluster.routing.allocation.node_concurrent_outgoing_recoveries=1] (can also be set via [cluster.routing.allocation.node_concurrent_recoveries])",decision.getExplanation());

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 22, 2018

Contributor

Nit: please add a space before the , separating the function arguments.

czjxy881 added some commits Feb 22, 2018

@czjxy881

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2018

I'm sorry, because my bash can't connect the internet,I must copy the code to the website. i forget to copy the import

@DaveCTurner
Copy link
Contributor

left a comment

Looking good, thanks. I asked for another handful of tiny changes.

@@ -299,6 +301,15 @@ public void testOutgoingThrottlesAllocation() {
new MoveAllocationCommand("test", 0, "node2", "node4")), true, false);
assertEquals(commandsResult.explanations().explanations().size(), 1);
assertEquals(commandsResult.explanations().explanations().get(0).decisions().type(), Decision.Type.THROTTLE);
boolean beingOutgoingThrottled = false;

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 22, 2018

Contributor

Could we call this foundThrottledMessage?

boolean beingOutgoingThrottled = false;
for (Decision decision : commandsResult.explanations().explanations().get(0).decisions().getDecisions()) {
if (decision.label().equals(ThrottlingAllocationDecider.NAME)) {
assertThat(decision.getExplanation(), containsString("node [node1] which holds the primary"));

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 22, 2018

Contributor

Could you assert the entire text of the message as you had done previously? It helps to clarify what this bit of code is testing.

beingOutgoingThrottled = true;
}
}
assertEquals(true, beingOutgoingThrottled);

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Feb 22, 2018

Contributor

Could you use assertTrue() here?

@DaveCTurner
Copy link
Contributor

left a comment

LGTM

@DaveCTurner

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

@elasticmachine please test this

@DaveCTurner DaveCTurner merged commit 497b3d7 into elastic:master Feb 22, 2018

2 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details

DaveCTurner added a commit that referenced this pull request Feb 22, 2018

Fix node ID reported by ThrottlingAllocationDecider (#28779)
Previously the message reported when `node_concurrent_outgoing_recoveries`
resulted in a `THROTTLE` decision included the reporting node's ID rather than
that of the primary. This commit fixes that.

Fixes #28777.
@DaveCTurner

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

Thanks for your contribution, @czjxy881. Merged as 497b3d7 and backported to 6.3 as fa59ecc.

@czjxy881 czjxy881 deleted the czjxy881:bug/ThrottlingAllocationDecider branch Feb 23, 2018

sebasjm pushed a commit to sebasjm/elasticsearch that referenced this pull request Mar 10, 2018

Fix node ID reported by ThrottlingAllocationDecider (elastic#28779)
Previously the message reported when `node_concurrent_outgoing_recoveries`
resulted in a `THROTTLE` decision included the reporting node's ID rather than
that of the primary. This commit fixes that.

Fixes elastic#28777.

@jpountz jpountz added the >bug label Jun 13, 2018

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 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.