search

devsisters / shardcake

Sharding and location transparency for Scala
https://devsisters.github.io/shardcake/
Apache License 2.0
395 stars 30 forks source link

Keep assignments got from shard manager in updateAssignments #135

Closed pancho-bo closed 3 months ago

pancho-bo commented 3 months ago

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 commented 3 months ago

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

pancho-bo commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Thanks for the catch!