[FLINK-8333] [flip6] Separate deployment options from command options #5220
Conversation
tillrohrmann
force-pushed
the
splitCliOptions
branch
2 times, most recently
from
1393089
to
a73b6e2
Compare
| * @param program The program for which to create the client. | ||
| * @throws Exception | ||
| */ | ||
| protected ClusterClient createClient( | ||
| CommandLineOptions options, | ||
| CustomCommandLine<?> customCommandLine, |
There was a problem hiding this comment.
Was it really necessary to change the signature? It seems that the behavior is otherwise the same. Why not leave the line
CustomCommandLine<?> activeCommandLine = getActiveCustomCommandLine(options.getCommandLine());
inside this method.
Edit: Is it needed in a later commit?
There was a problem hiding this comment.
Was probably not necessary. Since this method will be removed anyway in a subsequent PR, I'll keep it as it is.
| } | ||
| if (program != null) { | ||
| program.deleteExtractedLibraries(); | ||
| program.deleteExtractedLibraries(); |
There was a problem hiding this comment.
This won't run if createClient() fails. Before the client creation was inside the try block.
There was a problem hiding this comment.
Good catch. Will change it.
| catch (Exception e) { | ||
| return handleError(e); | ||
| } | ||
| jobId = new JobID(StringUtils.hexStringToByte(jobIdString)); |
There was a problem hiding this comment.
The exception's message won't be super meaningful:
new JobID(StringUtils.hexStringToByte("dupa"));
java.lang.NumberFormatException: For input string: "du"
There was a problem hiding this comment.
Good point. Will throw a more meaningful exception.
| LOG.error("Error: The value for the Job ID is not a valid ID."); | ||
| System.out.println("Error: The value for the Job ID is not a valid ID."); | ||
| return 1; | ||
| throw new CliArgsException("The value for the JobID is not a valid ID: " + e.getMessage()); |
There was a problem hiding this comment.
Maybe log the input value as well
try {
new JobID(StringUtils.hexStringToByte("dea84ec368d886f1638c01447ec6951"));
} catch (Exception e) {
throw new CliArgsException("The value for the JobID is not a valid ID: " + e.getMessage());
}
message is:
The value for the JobID is not a valid ID: Argument bytes must by an array of 16 bytes
Also, this piece of code is duplicated.
There was a problem hiding this comment.
Will change it.
| if (cleanedArgs.length >= 1) { | ||
| String jobIdString = cleanedArgs[0]; | ||
| try { | ||
| jobId = new JobID(StringUtils.hexStringToByte(jobIdString)); |
There was a problem hiding this comment.
Might as well use JobID.fromHexString() and add a meaningful message in fromHexString that you propagate.
| return triggerSavepoint(clusterClient, jobId, savepointDirectory); | ||
| } | ||
| } catch (Exception e) { | ||
| return handleError(e); |
There was a problem hiding this comment.
Why catch exception here? It's already done in parseParameters(String[] args).
There was a problem hiding this comment.
Oversight from my side.
| // Parse command line options | ||
| InfoOptions options; | ||
| try { | ||
| options = CliFrontendParser.parseInfoCommand(args); |
There was a problem hiding this comment.
CliFrontendParser.parseInfoCommand() is no longer in use. Can be deleted.
There was a problem hiding this comment.
Will remove it.
| // expected | ||
| } | ||
|
|
||
| Mockito.verify(testFrontend.client, times(1)).stop(any(JobID.class)); |
There was a problem hiding this comment.
You are already mocking the behavior of the client with:
doThrow(new IllegalArgumentException("Test exception")).when(client).stop(any(JobID.class));
Therefore the test should fail in the absence of the exception, i.e., if stop is not called. I think this verify is not needed:
@Test(expected = IllegalArgumentException.class)
public void testUnknownJobId() throws Exception {
// test unknown job Id
JobID jid = new JobID();
String[] parameters = {jid.toString()};
StopTestCliFrontend testFrontend = new StopTestCliFrontend(true);
testFrontend.stop(parameters);
fail("Should have failed.");
}
There was a problem hiding this comment.
True. Will adapt it accordingly.
| fail("Should have failed."); | ||
| } | ||
|
|
||
| @Test(expected = CliArgsException.class) |
There was a problem hiding this comment.
nit: Test could be stricter by asserting that the exception carries the expected message.
There was a problem hiding this comment.
True. I'm, however, not a super fan of testing for exception messages. If the message is changed then you don't see it directly.
| /** | ||
| * Tests for the RUN command. | ||
| */ | ||
| public class CliFrontendRunTest { |
There was a problem hiding this comment.
File is not recognized as moved by git 😢
There was a problem hiding this comment.
Sorry for that.
| int retCode = testFrontend.cancel(parameters); | ||
| assertTrue(retCode == 0); | ||
| int retCode = testFrontend.cancel(parameters); | ||
| assertTrue(retCode == 0); |
There was a problem hiding this comment.
nit: Code is copied but it's better to use assertEquals for proper failure reasons.
There was a problem hiding this comment.
Changes look good to me. Thanks for your contribution @tillrohrmann. I have added some comments that you may want to address.
tillrohrmann
force-pushed
the
splitCliOptions
branch
from
a73b6e2
to
db29504
Compare
|
Thanks a lot for your review @GJL. I've addressed your comments and rebased onto the preceding PR. Will merge it once Travis gives green light. |
…lusterClient Introduce YarnApplicationStatusMonitor which does the Yarn ApplicationStatus polling in the FlinkYarnSessionCli. This decouples the YarnClusterClient from the actual communication with Yarn and, thus, gives a better separation of concerns.
…f YarnClusterClient
…f YarnClusterClient
Moves the YarnClient from the YarnClusterClient to the AbstractYarnClusterDescriptor. This makes the latter responsible for the lifecycle management of the client and gives a better separation of concerns.
Move the savepoint disposal logic from the CliFrontend into the ClusterClient. This gives a better separation of concerns and allows the CliFrontend to be used with different ClusterClient implementations.
This commit separates the parsing of command options and deployment options into two steps. This makes it easier to make the CustomCommandLines non-static. Moreover, this commit moves the CliFrontend into the cli sub package.
tillrohrmann
force-pushed
the
splitCliOptions
branch
from
db29504
to
1806e04
Compare
What is the purpose of the change
This commit separates the parsing of command options and deployment options into two
steps. This makes it easier to make the CustomCommandLines non-static.
Moreover, this commit moves the CliFrontend into the cli sub package.
This PR is based on #5219.
Brief change log
CliFrontendtoorg.apache.flink.client.cliRunOptions,ListOptions, etc. fromCliFrontendParserVerifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation