Avoid unnecessary explanations in WriteLoadConstraintDecider

[DaveCTurner]

Today the WriteLoadConstraintDecider constructs various strings
explaining its decisions, but it only uses these messages if debug
logging is enabled or it's responding to an allocation explain request.
This commit skips the unnecessary work on the hot path.

[elasticsearchmachine]

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

[DiannaHohensee]

Maybe you could document

elasticsearch/server/src/main/java/org/elasticsearch/cluster/routing/allocation/RoutingAllocation.java

Lines 372 to 386 in a06527f

	/**
	* Create a routing decision, including the reason if the debug flag is
	* turned on
	* @param decision decision whether to allow/deny allocation
	* @param deciderLabel a human readable label for the AllocationDecider
	* @param reason a format string explanation of the decision
	* @param params format string parameters
	*/
	public Decision decision(Decision decision, String deciderLabel, String reason, Object... params) {
	if (debugDecision()) {
	return Decision.single(decision.type(), deciderLabel, reason, params);
	} else {
	return decision;
	}
	}

a bit more to suggest callers keep the input inexpensive if calling frequently, or call a debug check themselves? A warning that the cost often adds up quickly.

Perhaps a bit obvious, but I did it, so here we are :)

[DiannaHohensee]

Though Decision.single was used instead of the RoutingAllocation helper. That would suggest documentation / a warning in the Decision class 🤔

[DiannaHohensee]

LGTM, thanks for fixing it!

Left various suggestions with the aim of avoiding a repeat situation.

[DiannaHohensee]

allocation.decision will technically check the debug again, redundantly.

Maybe something more dramatic like allocation.decisionWithOptionalExplain would be more clear in the decision between RoutingAllocation#decision() and Decision#single() 😌

Anyway, I don't feel strongly about any of it.

[DaveCTurner]

It's not redundant, we could be here because debug logging is enabled but still we don't want to construct a new Decision when we can just return Decision.NOT_PREFERRED.

[DiannaHohensee]

Ah, you're right 👍

[DiannaHohensee]

Shouldn't this also have a if-else around allocation.debugDecision()?

Either for performance of not bothering with some of the string creation, or to model a consistent choice in the code for future readers.

I wonder if having RoutingAllocation#decision confuses things too much in the first place. What amount of string creation is OK vs not OK?

[DaveCTurner]

No need here, none of the parameters allocate anything new, they're just trivial accessor methods.

[DiannaHohensee]

Doesn't passing "Shard [%s] in index [%s] can be assigned to node [%s]. The node's utilization would become [%s]" as a parameter create a String?

[DaveCTurner]

No, string literals are constants, they're only allocated once. The bytecode that calls RoutingAllocation#decision just uses a simple ldc to push the string constant onto the stack, no allocation needed:

$ javap -p -c server/build/classes/java/main/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDecider.class | grep -A25 'utilization would become'
     526: ldc           #214                // String Shard [%s] in index [%s] can be assigned to node [%s]. The node\'s utilization would become [%s]
     528: iconst_4
     529: anewarray     #77                 // class java/lang/Object
     532: dup
     533: iconst_0
     534: aload_1
     535: invokevirtual #102                // Method org/elasticsearch/cluster/routing/ShardRouting.shardId:()Lorg/elasticsearch/index/shard/ShardId;
     538: aastore
     539: dup
     540: iconst_1
     541: aload_1
     542: invokevirtual #216                // Method org/elasticsearch/cluster/routing/ShardRouting.index:()Lorg/elasticsearch/index/Index;
     545: aastore
     546: dup
     547: iconst_2
     548: aload_2
     549: invokevirtual #122                // Method org/elasticsearch/cluster/routing/RoutingNode.nodeId:()Ljava/lang/String;
     552: aastore
     553: dup
     554: iconst_3
     555: fload         11
     557: invokestatic  #172                // Method java/lang/Float.valueOf:(F)Ljava/lang/Float;
     560: aastore
     561: invokevirtual #79                 // Method org/elasticsearch/cluster/routing/allocation/RoutingAllocation.decision:(Lorg/elasticsearch/cluster/routing/allocation/decider/Decision;Ljava/lang/String;Ljava/lang/String;[Ljava/lang/Object;)Lorg/elasticsearch/cluster/routing/allocation/decider/Decision;
     564: areturn

[DaveCTurner]

Added some docs in 50d1fb8.