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 10 commits into
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. |
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
LGTM with minor comments.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5006 +/- ##
============================================
+ Coverage 42.78% 43.33% +0.54%
+ Complexity 2197 2196 -1
============================================
Files 1045 1035 -10
Lines 39985 38856 -1129
Branches 4217 4064 -153
============================================
- Hits 17109 16839 -270
+ Misses 21818 20967 -851
+ Partials 1058 1050 -8
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Xiao-zhen-Liu Thank you for the comments. I added the fixes. |
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