Add a not-master state for desired balance

[ywangd]

The new state prevents a long running desired balance computation to set result after the node stands down as master.

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[elasticsearchmachine]

Hi @ywangd, I've created a changelog YAML for you.

[nicktindall]

Is this right? shouldn't we be using DesiredBalance.shardMovements(previousDesiredBalabnce, newDesiredBalance)? perhaps I'm not reading this right

[nicktindall]

Also for the diff above

[ywangd]

You are right. This unfortunately makes the code a bit ugly. Pushed 0b03f4e

[nicktindall]

Also the two lines above? updatedDesiredBalance -> oldDesiredBalance ?

[ywangd]

Yeah. Just noticed that. See a432406

[nicktindall]

Alternative approach? not sure if it's better or worse...

while (true) {
    final DesiredBalance previousBalance = currentDesiredBalanceRef.get();
    if (previousBalance == DesiredBalance.NOT_MASTER) {
        logger.debug("discard desired balance for [{}]", newDesiredBalance.lastConvergedIndex());
        break;
    }
    if (currentDesiredBalanceRef.compareAndSet(previousBalance, newDesiredBalance) {
        if (logger.isTraceEnabled()) {
            var diff = DesiredBalance.hasChanges(previousBalance, newDesiredBalance)
                ? "Diff: " + DesiredBalance.humanReadableDiff(previousBalance, newDesiredBalance)
                : "No changes";
            logger.trace("Desired balance updated: {}. {}", newDesiredBalance, diff);
        } else {
            logger.debug("Desired balance updated for [{}]", newDesiredBalance.lastConvergedIndex());
        }
        computedShardMovements.inc(DesiredBalance.shardMovements(previousBalance, newDesiredBalance));
        break;
    }
}

[nicktindall]

I think that's kind-of what updateAndGet does internally, because the javadoc says:

The function should be side-effect-free, since it may be re-applied when attempted updates fail due to contention among threads.

[ywangd]

That's viable as well. I don't have strong opinions. The concurrency here should be fairly low since it happens only during master failover.

[ywangd]

I think that's kind-of what updateAndGet does internally, because the javadoc says:

I think so too. We also use this pattern elsewhere in code.

I pushed b5cd708 to apply your suggeston.

I think it has an edge-case advantage to return earlier when the 1st read from currentDesiredBalanceRef is a not-master. The updateAndGet would probably try to set it again which may fail and trigger retry and see a different value which is from a newer term.

[nicktindall]

LGTM

[ywangd]

@elasticmachine update branch

[pxsalehi]

I find it a bit confusing that we have changed INITIAL to not be the initial state (at least that's what I understand from its name and usage). Maybe it deserves a different name and the two state could be clearly described in DesiredBalance?

[ywangd]

Fair point. I pushed 44cc48c to rename the two and add comments for both of them.

[pxsalehi]

How come both of these states are considered here?

[ywangd]

The first one, NOT_MASTER, is for when the node concurrently stands down as master. The 2nd one is for when the node concurrently stands down as master but then quickly elected as master again, which is very rare, but not impossible, especially in tests.

[pxsalehi]

do we really have no better alternative to this while loop here? It is not clear why we need to do this.

[pxsalehi]

(unless this is a performance critical code path, I'd personally prefer more synchronization to this non-blocking approach.)

[ywangd]

The while loop is basically what AtomicReference#updateAndGet does internally. I prefer it over synchronized for this particular case. The concurrency should not be high. But it is called in a cluster state applier thread on both standing down as master and becoming master. I'd like to avoid synchronization for such two usages. This field still needs to be volatile even with synchronization (unless we want to synchronize on read paths which seems bad). So overall I think it's better to have it as an AtomicReference with a bit spinning just on the computation thread. The complexity seems ok to me.

[pxsalehi]

I don't think setCurrentDesiredBalance gets called from the applier thread. I don't want to block this, feel free to merge it as is. I'm also not objecting to the correctness. I just think this is unnecessarily over-complicated.

[ywangd]

I meant currentDesiredBalanceRef is set on the applier thread, not this setCurrentDesiredBalance method which also sets currentDesiredBalanceRef. This method here is called only on the desired balance computation thread for which I think a bit spinning is fine. But I don't want the applier threads to have either spinning or synchronization.

Thanks for the discussion. Since you don't have strong objection (IIUC), I'd prefer to merge it as is. In my view, this method is really just a updateAndGet.

[ywangd]

@elasticmachine update branch