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
[FLINK-10099] [test] Rework YarnResourceManagerTest #6499
Conversation
9a93c27
to
0e849c5
Compare
0e849c5
to
3b93a6f
Compare
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.
Thanks for your contribution @tisonkun. The changes look good to me. I had some minor comments which we should address before merging. It would also be great if you could create a JIRA issue for this fix, since it is not super trivial. Please also update the commit accordingly.
@@ -258,7 +272,7 @@ protected void runAsync(final Runnable runnable) { | |||
rmServices.metricRegistry, | |||
rmServices.jobLeaderIdService, | |||
new ClusterInformation("localhost", 1234), | |||
fatalErrorHandler, | |||
testingFatalErrorHandler, |
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.
Nice one :-)
|
||
@Override | ||
public void setId(ContainerId containerId) { | ||
|
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.
Should we throw an UnsupportedOperationException
in order to be on the safe side if someone should ever call this method?
|
||
@Override | ||
public void setNodeId(NodeId nodeId) { | ||
|
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.
Same here with UnsupportedOperationException
|
||
@Override | ||
public Token getContainerToken() { | ||
return null; |
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.
Is it safe to return null
here?
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.
According to the ContainerPBImpl
it looks safe to do so.
|
||
@Override | ||
public void setContainerToken(Token token) { | ||
|
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.
UnsupportedOperationException
.
|
||
@Override | ||
public void setNodeHttpAddress(String s) { | ||
|
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.
UnsupportedOperationException
|
||
@Override | ||
public void setState(ContainerState containerState) { | ||
|
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.
UnsupportedOperationException
|
||
@Override | ||
public void setExitStatus(int exitStatus) { | ||
|
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.
UnsupportedOperationException
?
|
||
@Override | ||
public void setDiagnostics(String diagnostics) { | ||
|
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.
UnsupportedOperationException
?
@@ -415,13 +553,14 @@ public void testDeleteApplicationFiles() throws Exception { | |||
|
|||
resourceManager.deregisterApplication(ApplicationStatus.SUCCEEDED, null); | |||
assertFalse("YARN application directory was not removed", Files.exists(applicationDir.toPath())); | |||
|
|||
stopResourceManager(); |
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.
Maybe we could add a function
runTest(RunnableWithException testMethod) {
startResourceManager();
try {
testMethod.run()
} finally {
stopResourceManager
}
}
This method could then also be used by the testOnContainerCompleted
test.
rpcService.stopService().get(); | ||
} | ||
} | ||
|
||
static class TestingContainer extends Container { |
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.
I am not convinced about this. The mock was introduced with FLINK-8177 because Hadoop 2.9 added new methods.
See aa1f833#diff-73ea3caf58623db1fe68ef42ceadaa88L239
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.
Thanks for your contribution, @tisonkun. I am afraid that we cannot replace the container mock because a previous container implementation was removed for a reason: FLINK-8177
@tillrohrmann @GJL thanks for your reviews! |
dd4fc4d
to
b6b90e2
Compare
Thanks for addressing our comments @tisonkun. Merging this PR. |
Introduce methods to mock a Yarn Container and ContainerStatus. Properly shutdown a started ResourceManager. This closes apache#6499. (cherry picked from commit 3f40783)
What is the purpose of the change
to avoid OOM and gather mock stuff so that we can replace it once possible.
also some modifies to structure the test.
Brief change log
mockContainer
andmockContainerStatus
runTest
Verifying this change
the test is self-verified