fix: return -1 from getWorkerIndex for non-worker actor IDs#5006
fix: return -1 from getWorkerIndex for non-worker actor IDs#5006Ma77Ball wants to merge 2 commits intoapache:mainfrom
Conversation
|
/request-review @Yicong-Huang |
|
@Ma77Ball Let's try to assign PRs to other committers. @Xiao-zhen-Liu Please review it. |
|
@chenlica I've also used @aicam and @kunwp1. However, I get your point. I have used @aglinxinyuan and @Yicong-Huang a lot recently. Given the large codebase, it can be hard to know who to ask for PR reviews. It would be good to either:
I can raise an issue if you think this is an idea the project would like to pursue. |
|
Let's not make PR triage too complicated for now. We can have auto suggestions as an improvement down the road, but for now it's easier and better to do manual triage: If a requested reviewer knows who have a better context, simply reassign. Also, our code base is tiny, compared to other projects. You can usually do a git blame to find the owner/author of a component. If not, feel free to request @chenlica and he can find ppl to review. Let's not be blocked by some fancy auto triage method. |
|
In addition, I highly suggest everyone to review more PRs, even if that's not your experty. Share your thoughts, learn the code base, ask questions. For PR authors, feel free to request more ppl to get their attention.
Everyone should be comfortable review all codebase. Don't own only a file, or two. Don't be shy. I encourage everyone (not just committers) to contribute to and review everywhere. |
|
@Ma77Ball It will be easier for me to assign people to review PRs. Yes, we need more members to review PRs. |
What changes were proposed in this PR?
VirtualIdentityUtils.getWorkerIndex only matched the worker name pattern with no fallback case, so passing a non-worker ActorVirtualIdentity (e.g. CONTROLLER, SELF) threw scala.MatchError at runtime. This PR adds a fallback case that returns -1 for non-worker actor IDs, matching the graceful handling already present in the sibling methods getPhysicalOpId and toShorterString.
Any related issues, documentation, discussions?
Closes: #4727
How was this PR tested?
Updated VirtualIdentityUtilsSpec the existing test that pinned the MatchError behavior was replaced with a test asserting that getWorkerIndex returns -1 for special actor IDs like CONTROLLER. The existing test for the worker-name happy path still passes unchanged.
Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.7 in compliance with ASF