Fix 5667 - Changing use of sun NotImplementedException to apache NotImplementedException#5677
Fix 5667 - Changing use of sun NotImplementedException to apache NotImplementedException#5677ggdupont wants to merge 2 commits intoray-project:masterfrom
Conversation
…tImplementedException by using org.apache.commons.lang3.NotImplementedException instead
|
Test PASSed. |
kfstorm
left a comment
There was a problem hiding this comment.
Thanks for the catch. Looks good to me. One comment.
| @Override | ||
| public UniqueId getCurrentWorkerId() { | ||
| throw new NotImplementedException(); | ||
| throw new NotImplementedException("getCurrentWorkerId() not (yet) implemented."); |
There was a problem hiding this comment.
Suggest to change the reason to Not supported in SINGLE_PROCESS mode. as this is not a public API and it’s only referenced in RayNativeRuntime, which means we won’t implement it for single process mode.
…this API function is not used.
|
Test FAILed. |
| import org.ray.runtime.generated.Common.TaskType; | ||
| import org.ray.runtime.task.LocalModeTaskSubmitter; | ||
| import sun.reflect.generics.reflectiveObjects.NotImplementedException; | ||
| import org.apache.commons.lang3.NotImplementedException; |
There was a problem hiding this comment.
How about using java.lang.UnsupportedOperationException instead? The message does say "Not supported" so it's arguably the better message.
There was a problem hiding this comment.
Sounds good. I created another PR to replace all the NotImplementedExceptions. #5694
There was a problem hiding this comment.
Oops. Only one NotImplementedExceptions found after merging master (previously three). Either updating and merging this PR or merging #5694 is OK.
|
Can one of the admins verify this patch? |
Why are these changes needed?
See #5667
Basically, the use of sun.reflect.generics.reflectiveObjects.NotImplementedException creates a dependency to sun proprietary Class. It might be better suited to switch to another Class. In LocalModeRayletClient.java, it seems the apache lang version is preferred: org.apache.commons.lang3.NotImplementedException
Related issue number
Closes #5667
Checks
scripts/format.shto lint the changes in this PR.=> can't run it without installing yapf@0.23.0 which seems impossible now with brew
=> NA