zentol commented on a change in pull request #7717: [FLINK-11533] [container]
Add option parse JAR manifest for jobClassName
URL: https://github.com/apache/flink/pull/7717#discussion_r258959939
##########
File path:
flink-container/src/main/java/org/apache/flink/container/entrypoint/StandaloneJobClusterConfigurationParserFactory.java
##########
@@ -49,7 +49,9 @@
.required(true)
Review comment:
We'll probably need a tie-breaker here.
IMO,
* the `from-jar` approach isn't significantly harder to remove that a
dedicated option; I'd argue it would make a removal cleaner since we wouldn't
be removing a special case from an existing option.
* no additional argument is not a benefit in itself
* the argument checking is pretty much just
`commandLine.hasOption(op1.getOpt()) ^ commandLine.hasOption(op2.getOpt())`,
with a `FlinkParseException` being thrown if the result is false. (You might
argue that in order to do so we have to modify the exception signature of
`StandaloneJobClusterConfigurationParserFactory#createResult`, but we should do
so anyway as we are actually still parsing the rest port and job ID)
In any case we should split the `jobClassName` field to not have ambiguous
semantics; this could be modeled as an `Either<String, File>`; the `from-jar`
scheme is just a CLI construct to distinguish paths from class names; the
`ClassPathJobGraphRetriever ` shouldn't be aware of it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
