Fix double reincarnation #72
Conversation
…nds the reincarnation correctly back.
| status: 'suspect', | ||
| subjectIncNoDelta: -1 // send a gossip with an older incarnation number | ||
| }), | ||
| dsl.waitForPingResponse(t, tc, 0, 1, true), |
There was a problem hiding this comment.
Why the extra 2 arguments (1, true)? waitForPingResponse only expects three? (https://github.com/uber/ringpop-common/blob/5cc7e1dcdf5fa7feb8b8b625f15ac22b5ce781f9/test/ringpop-assert.js#L269)
There was a problem hiding this comment.
Ah, that is strange. I cheated a bit and took the base of this test from here: https://github.com/uber/ringpop-common/blob/fix/double-reincarnation/test/incarnation-no-tests.js#L34
I will remove it in this test 😃
|
one question but: LGTM! |
| @@ -896,7 +896,7 @@ function piggyback(tc, opts) { | |||
|
|
|||
| if(opts.subjectIx === 'sut') { | |||
| update.address = tc.sutHostPort; | |||
| update.sourceIncarnationNumber = tc.test_state['sutIncarnationNumber']; | |||
There was a problem hiding this comment.
Can you help me understand why this change is needed?
There was a problem hiding this comment.
It is hard to see without more context, but the piggyback function consists of roughly two parts. The first part is about the source of the gossip, the second part is about the subject. We are here in the part that is about the subject.
Even though we are setting the subject information this line had a bug where the incarnation number of the source of the gossip was set to the incarnation number of the sut. If you look at a bit more context below this line you will see that all branches set the incarnationNumber except for this branch.
There was a problem hiding this comment.
That makes sense. Thank you!
|
LGTM for the test, but see my question about the last line of the patch. |
|
LGTM |
This PR adds an integration tests assessing that reincarnation only happens for changes that would actually override the state of the node and don't reincarnate if the change will not result in an overridden state.