Fix race condition bug during local session repair (CASSANDRA-17335)

[bbotella]

Syncronizes session onSuccess handlePrepareSession for LocalSessions
patch by Bernardo Botella Corbi

[bereng]

This entry should go nder the 4.0.4 section

[bbotella]

Moving it

[bereng]

wdyt about overloading setStateAndSave() to have compare and set semantics? That would be a generalization of this one that could be more useful in the future.

[bbotella]

What do you suggest that should be compared inside the setStateAndSave() method? Would it be a fallback of the Preconditions.checkArgument checking the canTransitionTo of a given state, so instead of failing the method execution we just swallow it silently and log it?

If that's the suggestion, I see that the method is being used in several other places:

• handleFinalizeProposeMessage
• handleFinalizeCommitMessage
• handleStatusResponse
• cancelSession
• maybeSetRepairing

And not all of them have follow a strict compare and set pattern (some of them just have a set).

Having said that, if we wanted to overload setStateAndSave, my opinion is that it would be a slightly out of the scope change that may somehow force us to add fallbacks where we have currently none (those methods which only have a set).

I think it may require a bit of a broader discussion to basically answer this question: Are we sure we want to avoid failing fast on those cases? I am definitely open to debate.

[bereng]

The suggestion is to add to:

• setStateAndSave() and argument currentState that can be null. If null the method behaves as it does now. If a state is provided it will then compare state.getState() to currentState and fail or return false if they don't match. Rename the method to compareAndSaveStatei.e.
• Add a new setStateAndSavemethod without the currentState argument for all calls that don't need the compare behavior. It will just call compareAndSaveStatewith null.

No behavioral changes are introduced.

[yifan-c]

@bereng , understanding the motivation to generalize the CAS-like operation, I'd like to point out the logic here is mostly, if not in some state, then set and save. There is also a call-site that compares with 2 states.

        if (response.state == FINALIZED || response.state == FAILED)
        {
            setStateAndSave(session, response.state);
            logger.info("Unfinished local incremental repair session {} set to state {}", sessionID, response.state);
        }

[bereng]

Well if you guys don't like it I'm fine with it. It's just how I would do it but again, personal opinion, no big thing. +1