WriteLoadConstraintDecider: Don't require shard write load to return canRemain=NOT_PREFERRED

[nicktindall]

This might have been a hangover from before we did the work to prioritise shard movement. It's redundant now.

[elasticsearchmachine]

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

[ywangd]

LGTM

[ywangd]

Do we have a test to show a shard without write load can still trigger NOT_PREFERRED?

[nicktindall]

I will add one

[nicktindall]

Added in 6fc953f

[ywangd]

I assume we have a protection against non-existent shard write load in BalancedShardAllocator so that it is basically either ignored or ranked to the bottom when it comes to pick a movement to mitigate hotspot?

[nicktindall]

I assume we have a protection against non-existent shard write load in BalancedShardAllocator so that it is basically either ignored or ranked to the bottom when it comes to pick a movement to mitigate hotspot?

Yep:

elasticsearch/server/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java

Lines 1110 to 1122 in 09f5349

	final double lhsWriteLoad = shardWriteLoads.getOrDefault(lhs.shardId(), MISSING_WRITE_LOAD);
	final double rhsWriteLoad = shardWriteLoads.getOrDefault(rhs.shardId(), MISSING_WRITE_LOAD);
	// prefer any known write-load over any unknown write-load
	final var rhsIsMissing = rhsWriteLoad == MISSING_WRITE_LOAD;
	final var lhsIsMissing = lhsWriteLoad == MISSING_WRITE_LOAD;
	if (rhsIsMissing && lhsIsMissing) {
	return 0;
	}
	if (rhsIsMissing ^ lhsIsMissing) {
	return lhsIsMissing ? 1 : -1;
	}

[DiannaHohensee]

I don't understand why we'd want to remove this? Logically, if a shard has no write load, there's no benefit in moving it away. The decider seems more logically complete with the check.

The Allocator may not pass a null/0 value write load today, but there's no testing to protect against it in future, and then the behavior will be strange.

[ywangd]

if a shard has no write load, there's no benefit in moving it away.

My reasoning is that deciding whether the shard can remain and picking exactly which shard to move are two things. The later is done elsewhere which compares shard write loads already. So we don't need it for the can remain check. If for some reason, all shards reporting 0 write load incorrectly, we will still be able to move one of them with this change instead of taking no action.

there's no testing to protect against it in future

There is some protection as Nick commented here. We could also add a test for it.