fix(shard-distributor): return executor metadata for ephemeral shard assignments#7501
Conversation
…ormation Signed-off-by: Jakob Haahr Taankvist <jht@uber.com>
Signed-off-by: Jakob Haahr Taankvist <jht@uber.com>
jakobht
requested review from
3vilhamster,
Shaddoll,
davidporter-id-au,
demirkayaender,
dkrotx,
neil-xie,
sankari165,
shijiesheng and
taylanisikdemir
as code owners
| n.RUnlock() | ||
|
|
||
| if ok { | ||
| shardOwner, err := n.getShardOwnerInMap(ctx, n.shardOwners, shardID) |
There was a problem hiding this comment.
GetShardOwner and GetExecutor use the n.ShardOwners for the second argument, but shardID and executorID for the third argument. I think you wanted to use n.shardToExecutor. Would it be possible to cover it by tests, so that we expect GetShardOwner and GetExecutor use appropriate maps?
There was a problem hiding this comment.
However, I also don't see where n.shardToExecutor is populated...
There was a problem hiding this comment.
Good catch! It was acctually caught by tests, sadly CI was broken, I fixed it here: #7502
I have fixed it, and also updated the signature to take a pointer instead of a map, as refresh overwrites the variable
There was a problem hiding this comment.
However, I also don't see where n.shardToExecutor is populated...
It was already populated, just not saved on the struct. You can see where I changed shardToExecutor to n.shardToExecutor
…type issue Signed-off-by: Jakob Haahr Taankvist <jht@uber.com>
2 participants
What changed?
Modified ephemeral shard assignment to return executor metadata in the GetShardOwner response. Added GetExecutor method to retrieve executor information, including metadata.
Why?
When ephemeral shards were assigned to executors, the response only included the executor ID but not the metadata.
How did you test it?
Unit tests, and local testing
Potential risks
Release notes
Documentation Changes