Add node weight changes to balance round summaries

[DiannaHohensee]

The life cycle of the node weights during the balancer process:

1. BalancedShardsAllocator runs a round and places the node weights on the RoutingNodes copy
2. DesiredBalanceComputer then moves the node weights from the RoutingNodes to the DesiredBalance
3. Reconciliation then takes node weights from the DesiredBalance to the DesiredBalanceMetrics#update*() method.

So I'm grabbing the node weights from the DesiredBalance, comparing the previous and new DesiredBalance instances after a balancing round goes to update the current DesiredBalance reference.

I settled on summarizing the changes by saving the old DesiredBalance weights per node along with a weights diff to reach the new DesiredBalance's weights per node. This allows me to combine multiple summaries by using the oldest summary's base node weights, and summing the diffs across summaries to reach the final node weight diffs.

[elasticsearchmachine]

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

[DiannaHohensee]

I moved this into the BalancingRoundSummary class because I wanted to make it more obvious that both should be updated at the same time. I have not yet thought of a way to enforce the relationship.

[DiannaHohensee]

I've moved this logic into the AllocationBalancingRoundSummaryService to make it more unit testable. The summaries are also going to become more complex as I add more summary stats/metrics: that logic seems more appropriate in the summary service.

[nicktindall]

Looks good, just some minor comments, biggest concern is probably that delta in assertDoubleEquals

Again, sorry for the delay

[nicktindall]

Nit: I wonder if it's worth just changing the signature (and perhaps the name to reflect the change) of addBalancerRoundSummary to just take the old and new desired balances? then reduces some of the coupling between DesiredBalanceShardsAllocator and the AllocationBalancingRoundSummaryService?

[nicktindall]

I think you could do that and keep the testability (it could just call a static method on itself that could be exposed for testing)

[DiannaHohensee]

I'm not sure I understand your suggestion. If I change the method to

public void addBalancerRoundSummary(DesiredBalance oldDesired, DesiredBalance newDesired)

Then for testing addBalancerRoundSummary I'll have to create DesiredBalance instances and reason about the BalancingRoundSummary that results. Right now it's simpler to test addBalancerRoundSummary.

The createBalancerRoundSummary method could still be tested the same way even if it was internal (exposed for testing, as you say). But addBalancerRoundSummary seems more complicated, unless I'm missing something 🤔

[nicktindall]

I guess I just meant maybe it's easier, for the caller, to provide an overload e.g.

class AllocationBalancerRoundSummary {

   public void addBalancerRoundSummary(DesiredBalance old, DesiredBalance new) {
      addBalancerRoundSummary(createBalancerRoundSummary(old, new));
   }

   public void addBalancerRoundSummary(BalancingRoundSummary summary) {
      // ...
   }

   public static BalancingRoundSummary createBalancingRoundSummary(DesiredBalance old, DesiredBalance new) { 
      // ...
   }
}

That way DesiredBalanceShardAllocator doesn't need to know about createBalancingRoundSummary?

The overload being just a convenience that I'd argue doesn't need to be explicitly tested itself because it's just delegating to things that are tested.

But no strong feelings about this one. Up to you :)

[DiannaHohensee]

Oh I see, thanks for explaining. Sure, done in 3df5904

[DiannaHohensee]

Updated with the feedback: e99d3b9

I realized there was a little more cleanup I could do, per this comment. I also don't think I understand your suggestion in this other comment: could you take another look? @nicktindall

[DiannaHohensee]

I'm not sure I understand your suggestion. If I change the method to

public void addBalancerRoundSummary(DesiredBalance oldDesired, DesiredBalance newDesired)

Then for testing addBalancerRoundSummary I'll have to create DesiredBalance instances and reason about the BalancingRoundSummary that results. Right now it's simpler to test addBalancerRoundSummary.

The createBalancerRoundSummary method could still be tested the same way even if it was internal (exposed for testing, as you say). But addBalancerRoundSummary seems more complicated, unless I'm missing something 🤔

[nicktindall]

LGTM