-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-21006][TESTS][FOLLOW-UP]Some Worker's RpcEnv is leaked in WorkerSuite #18259
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
Conversation
|
@10110346 do you know of other related changes? we should make them all at once. |
|
Test build #77873 has finished for PR 18259 at commit
|
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.
Shouldn't this be override def after(): Unit = {?
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.
@srowen Because BeforeAndAfter is a trait,so we can simplify.
I referenced the 'MasterSuite.scala'
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.
That's bad practice, and why override exists. For example if you wrote afer { you'd maybe never notice that your cleanup code was silently not being triggered.
The code doesn't do this consistently, but more often does write the full declaration, which is probably a good idea. I'd recommend changing this one.
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.
@srowen I agree with you, i will change this one,thanks
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.
@srowen I have tried to change it, but after must write like this
|
@srowen Thanks. I have checked the whole project,there are no more related changes |
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.
That's bad practice, and why override exists. For example if you wrote afer { you'd maybe never notice that your cleanup code was silently not being triggered.
The code doesn't do this consistently, but more often does write the full declaration, which is probably a good idea. I'd recommend changing this one.
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.
Space before
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.
Space before for readability
|
Test build #77929 has finished for PR 18259 at commit
|
|
Test build #77955 has finished for PR 18259 at commit
|
|
Test build #77964 has started for PR 18259 at commit |
|
Jenkins, retest this please |
|
Test build #77980 has finished for PR 18259 at commit
|
|
Merged to master |
…kerSuite ## What changes were proposed in this pull request? Create rpcEnv and run later needs shutdown. as apache#18226 ## How was this patch tested? unit test Author: liuxian <[email protected]> Closes apache#18259 from 10110346/wip-lx-0610.
What changes were proposed in this pull request?
Create rpcEnv and run later needs shutdown. as #18226
How was this patch tested?
unit test