Keep assignments got from shard manager in updateAssignments

[pancho-bo]

It seems that here client will throw away assignments got from shard manager if there are any assignments already. Not sure if that was intended.

[ghostdogpr]

The assignments from Shard Manager contain the full map of assignments, so we can simply replace it.

[pancho-bo]

The assignments from Shard Manager contain the full map of assignments, so we can simply replace it.

Does it mean that previous shardAssigments could be discarded altogether?

[ghostdogpr]

Does it mean that previous shardAssigments could be discarded altogether?

Yeah, I think I added the if (map.isEmpty) assignments else map as a defensive measure in case of weird behavior such as maybe calling the shard manager while it's loading or in a weird state. Basically an empty map is unexpected so we don't do anything in this case.

[pancho-bo]

You probably mean that a non-empty map is unexpected, as sharding-client will refresh assignments during startup. And the first thing it will receive should be shard-manager response with its current state.
But there is another call to updateAssignments from the shard-manager, in the handle PodUnavailable error case. Calling updateAssignments at that time will have a non-empty current assignments and will ignore shard-manager response. That means that error will persist for some more time. Correct me if my understanding is wrong. I will try to come up with test case.

[ghostdogpr]

shardManager.getAssignments calls the shard manager directly, so it should be 100% reliable if it returns a response, as opposed to storage.getAssignments/storage.assignmentsStream which might be outdated/empty/miss something because it relies on Redis. That's why when a pod is unavailable, we try to get the latest state from the Shard Manager in case there was a storage issue. That value will be correct so it's okay to replace whatever we had before.

[pancho-bo]

shardManager.getAssignments calls the shard manager directly, so it should be 100% reliable if it returns a response, as opposed to storage.getAssignments/storage.assignmentsStream which might be outdated/empty/miss something because it relies on Redis. That's why when a pod is unavailable, we try to get the latest state from the Shard Manager in case there was a storage issue. That value will be correct so it's okay to replace whatever we had before.

Agree. What I am trying to convey is that the code if (map.isEmpty) assignments else map is seeming to do exactly opposite, as it will keep the current, probably corrupted, map and discard received from shard manager, correct, assignments.

[ghostdogpr]

Ah, you are right, it's doing the opposite 🤔
I think maybe initially we were trying to set it only once, but now that we are calling it in handleError it doesn't make sense anymore.

[ghostdogpr]

Thanks for the catch!