[FLINK-9499][rest] Support JSON request in JarHandlers #6330
Conversation
|
Currently preparing the documentation changes. |
| * {@link RequestBody} for running a jar. | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public class JarRunRequestBody implements RequestBody { |
There was a problem hiding this comment.
missing marshalling test
There was a problem hiding this comment.
Thanks for your contribution @zentol. LGTM. I had some minor comments which we could address before merging this PR.
| () -> getQueryParameter(request, AllowNonRestoredStateQueryParameter.class), | ||
| false, | ||
| log); | ||
| final String savepointPath = fromRequestBodyOrQueryParameter( |
There was a problem hiding this comment.
Maybe we could add a fromRequestBodyOrQueryParameter method which does not take a default value as a convenience function. That way we would not have all the calls where we pass null.
There was a problem hiding this comment.
This could result in unexpected NullPointerExceptions when retrieving a primitive, like in the following example:
fromRequestBodyOrQueryParameter(
requestBody.getParallelism(),
() -> getQueryParameter(request, ParallelismQueryParameter.class)
log);
The explicit default argument prevents that from happening.
There was a problem hiding this comment.
Wouldn't something like this work:
private static <T> T fromRequestBodyOrQueryParameter(
T requestValue,
SupplierWithException<T, RestHandlerException> queryParameterExtractor,
Logger log) throws RestHandlerException {
return fromRequestBodyOrQueryParameter(requestValue, queryParameterExtractor, null, log);
}
There was a problem hiding this comment.
How does this prevent the scenario i described?
There was a problem hiding this comment.
True, making the specification of the default value explicit will better guard against this. In the other case, the user would have to know the difference between these methods and which to apply to primitive and non-primitive values. Alright, then it's good to go from my side.
| @JsonProperty(FIELD_NAME_PROGRAM_ARGUMENTS) String programArguments, | ||
| @JsonProperty(FIELD_NAME_PARALLELISM) Integer parallelism, | ||
| @JsonProperty(FIELD_NAME_ALLOW_NON_RESTORED_STATE) Boolean allowNonRestoredState, | ||
| @JsonProperty(FIELD_NAME_SAVEPOINT_PATH) String savepointPath) { |
There was a problem hiding this comment.
If the fields are @Nullable shouldn't the constructor parameters be also @Nullable? Or are the fields only @Nullable because of the default constructor?
There was a problem hiding this comment.
yes they should be nullable
There was a problem hiding this comment.
For example, if only a partial body is sent some fields may be null. I couldn't quickly find a way to allow either all fields or non to be null.
| /** | ||
| * Tests for the parameter handling of the {@link JarRunHandler}. | ||
| */ | ||
| public class JarRunHandlerParameterTest { |
There was a problem hiding this comment.
Should extend TestLogger.
|
|
||
| return new HandlerRequest<>( | ||
| requestBody, | ||
| JarRunHeaders.getInstance().getUnresolvedMessageParameters(), |
There was a problem hiding this comment.
Can't we simply give the parameters parameter here instead of creating a new JarRunMessageParameters instance? Then we could also avoid having to create the queryParametersAsMap map.
There was a problem hiding this comment.
HandlerRequest doesn't work like that.
It takes the parameter map from netty and inserts them into the MessageParameters.
We could move logic into a static factory method and have the request be a simple container, but changing that would be out-of-scope of this PR.
There was a problem hiding this comment.
Ah I see. You're right. I think this could be a nice improvement to initialize the MessageParameter instance before giving it to the HandlerRequest. But this is out of scope for this PR.
What is the purpose of the change
With this PR it is now possible to configure the submission via the
JarRunHandlerwith a JSON payload instead of query parameters.This has a number of advantages:
The handler now accepts the configuration both via JSON and query parameters, with JSON being prioritized. The WebUI makes use of this to be compatible with both new and legacy handlers, which is pretty rad.
Note that the
JarPlanHandlerstill suffers from the problems listed above. While this handler technically works similar to theJarRunHandler(in fact we could adjust it to work the same way) it unfortunately is aGETrequest, for which we do not allow payloads.This PR shares some code with #6311.
BlobServerResourceBrief change log
JarRunRequestBody, with optional fields for all parametersJarRunHandlerto accept parameters both via JSON and query parametersVerifying this change
This change added tests and can be verified as follows:
flink-runtime-web(this step is important since we build jar for the test)JarRunHandlerParameterTestThe test verifies the successful submission with parameters
Additionally, start a local cluster and manually submit jobs.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes)Documentation