-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-46353][CORE] Refactor to improve RegisterWorker
unit test coverage
#44284
Conversation
ac7db76
to
f617630
Compare
RegisterWorker
unit testRegisterWorker
unit test coverage
val masterAddress: RpcAddress = rw.masterAddress | ||
val resources: Map[String, ResourceInformation] = rw.resources | ||
|
||
logInfo("Registering worker %s:%d with %d cores, %s RAM".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following is a copy&paste
of the existing code.
cc @viirya |
Thank you so much, @viirya ! |
33ac937
to
ad6f845
Compare
b858ec1
to
284b019
Compare
Hi, @viirya . Could you review once more? The following is the change from the original refactoring PR which you approved. - handleRegisterWorker(registerWorker)
+ handleRegisterWorker(id, workerHost, workerPort, workerRef, cores, memory, workerWebUiUrl,
+ masterAddress, resources) |
@@ -676,6 +652,45 @@ private[deploy] class Master( | |||
logInfo(f"Recovery complete in ${timeTakenNs / 1000000000d}%.3fs - resuming operations!") | |||
} | |||
|
|||
private[master] def handleRegisterWorker( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean this method's parameters change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But its code is still the same right? (copied from existing code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the code is the same. I change the parameter of handleRegisterWorker
method from a single RegisterWorker
class to field parameters to avoid side-effects.
Thank you so much, @viirya . I'll dig more this area with more test cases. |
…verage This PR aims to improve the unit test coverage for `RegisterWorker` message handling. - Add `handleRegisterWorker` helper method which is testable easily. - Add new unit tests for three conditional branches. It's easily to test and improve. We can add more tests in this way in the future. No. This is a refactoring on the main code and only additions to the test methods. Pass the CIs. No. Closes apache#44284 from dongjoon-hyun/SPARK-46353. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
What changes were proposed in this pull request?
This PR aims to improve the unit test coverage for
RegisterWorker
message handling.handleRegisterWorker
helper method which is testable easily.Why are the changes needed?
It's easily to test and improve. We can add more tests in this way in the future.
Does this PR introduce any user-facing change?
No. This is a refactoring on the main code and only additions to the test methods.
How was this patch tested?
Pass the CIs.
Was this patch authored or co-authored using generative AI tooling?
No.