1996fanrui commented on code in PR #21840:
URL: https://github.com/apache/flink/pull/21840#discussion_r1101110410
##########
flink-core/src/main/java/org/apache/flink/api/common/ArchivedExecutionConfig.java:
##########
@@ -44,6 +45,11 @@ public class ArchivedExecutionConfig implements Serializable
{
public ArchivedExecutionConfig(ExecutionConfig ec) {
executionMode = ec.getExecutionMode().name();
+ if (ec.getJobType() != null) {
+ jobType = ec.getJobType();
+ } else {
+ jobType = "UNKNOWN";
Review Comment:
Is it possible for `ec.getJobType()` to be null?
I see `ec.getJobType() = configuration.get(JOB_TYPE);`, and the default
value of `JOB_TYPE` is `STREAMING`.
##########
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java:
##########
@@ -541,10 +549,12 @@ public ExecutionConfig setExecutionRetryDelay(long
executionRetryDelay) {
* Sets the execution mode to execute the program. The execution mode
defines whether data
* exchanges are performed in a batch or on a pipelined manner.
*
- * <p>The default execution mode is {@link ExecutionMode#PIPELINED}.
- *
+ * @deprecated This configuration is used in the deprecated DataSet API.
The {@link
+ * ExecutionConfig#setJobType(String)} sets execution runtime mode of
job
+ * <p>The default execution mode is {@link ExecutionMode#PIPELINED}.
* @param executionMode The execution mode to use.
*/
+ @Deprecated
Review Comment:
Add the `@Deprecated` for EXECUTION_MODE should be a separate commit.
You can take a look the commit part of `Code Style and Quality Guide`[1].
[1]
https://flink.apache.org/contributing/code-style-and-quality-pull-requests.html#separate-refactoring-cleanup-and-independent-changes
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/handler/legacy/utils/ArchivedExecutionConfigBuilder.java:
##########
@@ -75,6 +81,7 @@ public ArchivedExecutionConfigBuilder
setPeriodicMaterializeIntervalMillis(
public ArchivedExecutionConfig build() {
return new ArchivedExecutionConfig(
executionMode != null ? executionMode :
ExecutionMode.PIPELINED.name(),
+ jobType != null ? jobType : "UNKNOWN",
Review Comment:
If using the `UNKNOWN` as the default jobType here, why set `STREAMING` as
the default value for `ConfigOption<String> JOB_TYPE`?
##########
flink-core/src/main/java/org/apache/flink/api/common/ArchivedExecutionConfig.java:
##########
@@ -68,7 +74,28 @@ public ArchivedExecutionConfig(
boolean objectReuseEnabled,
long periodicMaterializeIntervalMillis,
Map<String, String> globalJobParameters) {
+ this(
+ executionMode,
+ "UNKNOW",
+ restartStrategyDescription,
+ maxParallelism,
+ parallelism,
+ objectReuseEnabled,
+ periodicMaterializeIntervalMillis,
+ globalJobParameters);
+ }
+
+ public ArchivedExecutionConfig(
+ String executionMode,
+ String jobType,
+ String restartStrategyDescription,
+ int maxParallelism,
+ int parallelism,
+ boolean objectReuseEnabled,
+ long periodicMaterializeIntervalMillis,
+ Map<String, String> globalJobParameters) {
Review Comment:
Why do you add a constructor? could we reuse the last constructor and add
the `String jobType`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
