-
Notifications
You must be signed in to change notification settings - Fork 31
Remove broken host map subset assertions from nodetool #333
Conversation
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure on how this works, so am not able to effectively review it.
It'd be great to also see an e2e test added with this that tests the scenario described in #331. I'm apprehensive to close the issue without knowing for sure it's resolved 😄
leavingNodes, joiningNodes, movingNodes, mappedNodes, | ||
) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function confuses me a lot, and I'm not really sure how it works.
Can you add a comment to the function itself explaining the rough algorithm in place here, so that this change can be reviewed properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've documented the function. Please take another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks a lot. Makes sense and lgtm 😃
797e20f
to
9f57da1
Compare
I've added a docstring to the function as you suggested.
I doubt I'm going to be able to write an E2E test for this particular failure. |
/retest |
Would simply ensuring we can add a new node to an existing cluster without any of the other nodes failing their readiness probes test this case? |
9f57da1
to
b5d9301
Compare
/retest |
Ok. Looks like I triggered the failure:
Unfortunately, the pilot logs aren't available because the container we restarted by the subsequent liveness probe test. Perhaps I'll change the tests to exit after the first failure. |
b5d9301
to
f003c65
Compare
Here we go. The test failed and the logs contain the nodetool error
It only failed on the 1.7 cluster. Now I'll commit the fix and expect the tests to pass consistently. |
d6e441b
to
d108389
Compare
…nformation related to the failure
d108389
to
66c66d4
Compare
66c66d4
to
a052688
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: munnerz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes: #331
Release note: