[GitHub] [flink] flinkbot edited a comment on issue #9013: [FLINK-13136] Fix documentation error about stopping job with restful api
flinkbot edited a comment on issue #9013: [FLINK-13136] Fix documentation error about stopping job with restful api URL: https://github.com/apache/flink/pull/9013#issuecomment-509065631 Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community to review your pull request. We will use this comment to track the progress of the review. ## Automated Checks Last check on commit 097af583c8040c16da17b796f6e4060c270b5b1d (Thu Sep 12 07:04:29 UTC 2019) **Warnings:** * No documentation files were touched! Remember to keep the Flink docs up to date! Mention the bot in a comment to re-run the automated checks. ## Review Progress * ❓ 1. The [description] looks good. * ❓ 2. There is [consensus] that the contribution should go into to Flink. * ❓ 3. Needs [attention] from. * ❓ 4. The change fits into the overall [architecture]. * ❓ 5. Overall code [quality] is good. Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands The @flinkbot bot supports the following commands: - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`) - `@flinkbot approve all` to approve all aspects - `@flinkbot approve-until architecture` to approve everything until `architecture` - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention - `@flinkbot disapprove architecture` to remove an approval you gave earlier 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] GJL commented on a change in pull request #9013: [FLINK-13136] Fix documentation error about stopping job with restful api
GJL commented on a change in pull request #9013: [FLINK-13136] Fix
documentation error about stopping job with restful api
URL: https://github.com/apache/flink/pull/9013#discussion_r323583397
##
File path:
flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java
##
@@ -375,7 +374,7 @@ public JobSubmissionResult submitJob(JobGraph jobGraph,
ClassLoader classLoader)
public void cancel(JobID jobID) throws Exception {
JobCancellationMessageParameters params = new
JobCancellationMessageParameters();
params.jobPathParameter.resolve(jobID);
-
params.terminationModeQueryParameter.resolve(Collections.singletonList(TerminationModeQueryParameter.TerminationMode.CANCEL));
+
params.terminationModeQueryParameter.resolve(Collections.singletonList("CANCEL"));
Review comment:
I liked using the enum `TerminationModeQueryParameter.TerminationMode`
better because now we have to repeat String constants. The deprecation of
`TerminationModeQueryParameter` and `TerminationMode` was wrong and should be
reverted. What should be deprecated is `TerminationMode.STOP` since
`TerminationMode.CANCEL` is still valid for non-checkpointed and batch jobs.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] GJL commented on a change in pull request #9013: [FLINK-13136] Fix documentation error about stopping job with restful api
GJL commented on a change in pull request #9013: [FLINK-13136] Fix
documentation error about stopping job with restful api
URL: https://github.com/apache/flink/pull/9013#discussion_r323583397
##
File path:
flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java
##
@@ -375,7 +374,7 @@ public JobSubmissionResult submitJob(JobGraph jobGraph,
ClassLoader classLoader)
public void cancel(JobID jobID) throws Exception {
JobCancellationMessageParameters params = new
JobCancellationMessageParameters();
params.jobPathParameter.resolve(jobID);
-
params.terminationModeQueryParameter.resolve(Collections.singletonList(TerminationModeQueryParameter.TerminationMode.CANCEL));
+
params.terminationModeQueryParameter.resolve(Collections.singletonList("CANCEL"));
Review comment:
I liked using the enum `TerminationModeQueryParameter.TerminationMode`
better because now we have to repeat String constants. The deprecation of
`TerminationModeQueryParameter` and `TerminationMode` was wrong and should be
reverted. What should be deprecated is `TerminationMode.STOP` since
`TerminationMode.CANCEL` is still valid for non-checkpointed jobs and batch
jobs.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy…
zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime]
Implement new generation restart strategy loader which also respects legacy…
URL: https://github.com/apache/flink/pull/8912#discussion_r323583982
##
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/executiongraph/failover/flip1/RestartBackoffTimeStrategyFactoryLoaderTest.java
##
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.executiongraph.failover.flip1;
+
+import org.apache.flink.api.common.restartstrategy.RestartStrategies;
+import org.apache.flink.api.common.time.Time;
+import org.apache.flink.configuration.ConfigConstants;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.RestartBackoffTimeStrategyOptions;
+import org.apache.flink.util.TestLogger;
+
+import org.junit.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.core.IsInstanceOf.instanceOf;
+
+/**
+ * Unit tests for {@link RestartBackoffTimeStrategyFactoryLoader}.
+ */
+public class RestartBackoffTimeStrategyFactoryLoaderTest extends TestLogger {
+
+ private static final RestartStrategies.RestartStrategyConfiguration
DEFAULT_RESTART_STRATEGY_CONFIGURATION =
+ new RestartStrategies.FallbackRestartStrategyConfiguration();
+
+ @Test
+ public void testNewStrategySpecified() throws Exception {
+ // specify RestartBackoffTimeStrategy directly in cluster config
+ final Configuration conf = new Configuration();
+ conf.setString(
+
RestartBackoffTimeStrategyOptions.RESTART_BACKOFF_TIME_STRATEGY_CLASS_NAME,
+ TestRestartBackoffTimeStrategy.class.getName());
+
+ // the RestartStrategyConfiguration should not take effect as
the loader will
+ // directly create the factory from the config of the new
version strategy
+ final RestartBackoffTimeStrategy.Factory factory =
+
RestartBackoffTimeStrategyFactoryLoader.createRestartStrategyFactory(
+ new
RestartStrategies.FailureRateRestartStrategyConfiguration(
+ 1,
+ Time.milliseconds(1000),
+ Time.milliseconds(1000)),
+ conf,
+ true);
+
+ assertThat(
+ factory,
+
instanceOf(TestRestartBackoffTimeStrategy.TestRestartBackoffTimeStrategyFactory.class));
+ }
+
+ @Test
+ public void testInvalidNewStrategySpecified() throws Exception {
+ final Configuration conf = new Configuration();
+ conf.setString(
+
RestartBackoffTimeStrategyOptions.RESTART_BACKOFF_TIME_STRATEGY_CLASS_NAME,
+ InvalidTestRestartBackoffTimeStrategy.class.getName());
+
+ final RestartBackoffTimeStrategy.Factory factory =
+
RestartBackoffTimeStrategyFactoryLoader.createRestartStrategyFactory(
+ DEFAULT_RESTART_STRATEGY_CONFIGURATION,
+ conf,
+ true);
+
+ assertThat(
+ factory,
+
instanceOf(NoRestartBackoffTimeStrategy.NoRestartBackoffTimeStrategyFactory.class));
+ }
+
+ @Test
+ public void testNoStrategySpecifiedWhenCheckpointingEnabled() throws
Exception {
+ final RestartBackoffTimeStrategy.Factory factory =
+
RestartBackoffTimeStrategyFactoryLoader.createRestartStrategyFactory(
+ DEFAULT_RESTART_STRATEGY_CONFIGURATION,
+ new Configuration(),
+ true);
+
+ assertThat(
+ factory,
+
instanceOf(FixedDelayRestartBackoffTimeStrategy.FixedDelayRestartBackoffTimeStrategyFactory.class));
+ }
+
+ @Test
+ public void
[GitHub] [flink] GJL commented on a change in pull request #9013: [FLINK-13136] Fix documentation error about stopping job with restful api
GJL commented on a change in pull request #9013: [FLINK-13136] Fix
documentation error about stopping job with restful api
URL: https://github.com/apache/flink/pull/9013#discussion_r323583397
##
File path:
flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java
##
@@ -375,7 +374,7 @@ public JobSubmissionResult submitJob(JobGraph jobGraph,
ClassLoader classLoader)
public void cancel(JobID jobID) throws Exception {
JobCancellationMessageParameters params = new
JobCancellationMessageParameters();
params.jobPathParameter.resolve(jobID);
-
params.terminationModeQueryParameter.resolve(Collections.singletonList(TerminationModeQueryParameter.TerminationMode.CANCEL));
+
params.terminationModeQueryParameter.resolve(Collections.singletonList("CANCEL"));
Review comment:
I liked using the enum `TerminationModeQueryParameter.TerminationMode`
better. The deprecation of `TerminationModeQueryParameter` and
`TerminationMode` was wrong and should be reverted. What should be deprecated
is `TerminationMode.STOP` since `TerminationMode.CANCEL` is still valid for
non-checkpointed and batch jobs.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy…
flinkbot edited a comment on issue #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy… URL: https://github.com/apache/flink/pull/8912#issuecomment-506306353 Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community to review your pull request. We will use this comment to track the progress of the review. ## Automated Checks Last check on commit 1bd21c6cd2d430b9827b903e80f31c55bcb04750 (Thu Sep 12 07:03:28 UTC 2019) **Warnings:** * No documentation files were touched! Remember to keep the Flink docs up to date! Mention the bot in a comment to re-run the automated checks. ## Review Progress * ❓ 1. The [description] looks good. * ❓ 2. There is [consensus] that the contribution should go into to Flink. * ❓ 3. Needs [attention] from. * ❓ 4. The change fits into the overall [architecture]. * ❓ 5. Overall code [quality] is good. Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands The @flinkbot bot supports the following commands: - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`) - `@flinkbot approve all` to approve all aspects - `@flinkbot approve-until architecture` to approve everything until `architecture` - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention - `@flinkbot disapprove architecture` to remove an approval you gave earlier 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy…
zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime]
Implement new generation restart strategy loader which also respects legacy…
URL: https://github.com/apache/flink/pull/8912#discussion_r323583478
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/failover/flip1/RestartBackoffTimeStrategyFactoryLoader.java
##
@@ -0,0 +1,393 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.executiongraph.failover.flip1;
+
+import org.apache.flink.api.common.restartstrategy.RestartStrategies;
+import org.apache.flink.configuration.AkkaOptions;
+import org.apache.flink.configuration.ConfigConstants;
+import org.apache.flink.configuration.ConfigOption;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.RestartBackoffTimeStrategyOptions;
+import
org.apache.flink.runtime.executiongraph.restart.FixedDelayRestartStrategy;
+import org.apache.flink.runtime.executiongraph.restart.NoRestartStrategy;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.concurrent.TimeUnit;
+
+import scala.concurrent.duration.Duration;
+
+import static org.apache.flink.util.Preconditions.checkArgument;
+import static org.apache.flink.util.Preconditions.checkNotNull;
+
+/**
+ * A utility class to load {@link RestartBackoffTimeStrategy.Factory} from the
configuration.
+ */
+public class RestartBackoffTimeStrategyFactoryLoader {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RestartBackoffTimeStrategyFactoryLoader.class);
+
+ private static final String CREATE_METHOD = "createFactory";
+
+ /**
+* Creates proper {@link RestartBackoffTimeStrategy.Factory}.
+* If new version restart strategy is specified, will directly use it.
+* Otherwise will decide based on legacy restart strategy configs.
+*
+* @param jobRestartStrategyConfiguration restart configuration given
within the job graph
+* @param clusterConfiguration cluster(server-side) configuration,
usually represented as jobmanager config
+* @param isCheckpointingEnabled if checkpointing was enabled for the
job
+* @return new version restart strategy factory
+*/
+ public static RestartBackoffTimeStrategy.Factory
createRestartStrategyFactory(
+ final RestartStrategies.RestartStrategyConfiguration
jobRestartStrategyConfiguration,
+ final Configuration clusterConfiguration,
+ final boolean isCheckpointingEnabled) throws Exception {
+
+ checkNotNull(jobRestartStrategyConfiguration);
+ checkNotNull(clusterConfiguration);
+
+ final String restartStrategyClassName =
clusterConfiguration.getString(
+
RestartBackoffTimeStrategyOptions.RESTART_BACKOFF_TIME_STRATEGY_CLASS_NAME);
+
+ if (restartStrategyClassName != null) {
+ // create new restart strategy directly if it is
specified in cluster config
+ return
createRestartStrategyFactoryInternal(clusterConfiguration);
+ } else {
+ // adapt the legacy restart strategy configs as new
restart strategy configs
+ final Configuration adaptedConfiguration =
getAdaptedConfiguration(
+ jobRestartStrategyConfiguration,
+ clusterConfiguration,
+ isCheckpointingEnabled);
+
+ // create new restart strategy from the adapted config
+ return
createRestartStrategyFactoryInternal(adaptedConfiguration);
+ }
+ }
+
+ /**
+* Decides the {@link RestartBackoffTimeStrategy} to use and its params
based on legacy configs,
+* and records its class name and the params into a adapted
configuration.
+*
+* The decision making is as follows:
+*
+* Use strategy of {@link
[GitHub] [flink] flinkbot edited a comment on issue #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy…
flinkbot edited a comment on issue #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy… URL: https://github.com/apache/flink/pull/8912#issuecomment-506306353 Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community to review your pull request. We will use this comment to track the progress of the review. ## Automated Checks Last check on commit 1bd21c6cd2d430b9827b903e80f31c55bcb04750 (Thu Sep 12 07:02:27 UTC 2019) **Warnings:** * No documentation files were touched! Remember to keep the Flink docs up to date! Mention the bot in a comment to re-run the automated checks. ## Review Progress * ❓ 1. The [description] looks good. * ❓ 2. There is [consensus] that the contribution should go into to Flink. * ❓ 3. Needs [attention] from. * ❓ 4. The change fits into the overall [architecture]. * ❓ 5. Overall code [quality] is good. Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands The @flinkbot bot supports the following commands: - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`) - `@flinkbot approve all` to approve all aspects - `@flinkbot approve-until architecture` to approve everything until `architecture` - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention - `@flinkbot disapprove architecture` to remove an approval you gave earlier 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy…
zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime]
Implement new generation restart strategy loader which also respects legacy…
URL: https://github.com/apache/flink/pull/8912#discussion_r323583514
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/failover/flip1/RestartBackoffTimeStrategyFactoryLoader.java
##
@@ -0,0 +1,393 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.executiongraph.failover.flip1;
+
+import org.apache.flink.api.common.restartstrategy.RestartStrategies;
+import org.apache.flink.configuration.AkkaOptions;
+import org.apache.flink.configuration.ConfigConstants;
+import org.apache.flink.configuration.ConfigOption;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.RestartBackoffTimeStrategyOptions;
+import
org.apache.flink.runtime.executiongraph.restart.FixedDelayRestartStrategy;
+import org.apache.flink.runtime.executiongraph.restart.NoRestartStrategy;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.concurrent.TimeUnit;
+
+import scala.concurrent.duration.Duration;
+
+import static org.apache.flink.util.Preconditions.checkArgument;
+import static org.apache.flink.util.Preconditions.checkNotNull;
+
+/**
+ * A utility class to load {@link RestartBackoffTimeStrategy.Factory} from the
configuration.
+ */
+public class RestartBackoffTimeStrategyFactoryLoader {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RestartBackoffTimeStrategyFactoryLoader.class);
+
+ private static final String CREATE_METHOD = "createFactory";
+
+ /**
+* Creates proper {@link RestartBackoffTimeStrategy.Factory}.
+* If new version restart strategy is specified, will directly use it.
+* Otherwise will decide based on legacy restart strategy configs.
+*
+* @param jobRestartStrategyConfiguration restart configuration given
within the job graph
+* @param clusterConfiguration cluster(server-side) configuration,
usually represented as jobmanager config
+* @param isCheckpointingEnabled if checkpointing was enabled for the
job
+* @return new version restart strategy factory
+*/
+ public static RestartBackoffTimeStrategy.Factory
createRestartStrategyFactory(
+ final RestartStrategies.RestartStrategyConfiguration
jobRestartStrategyConfiguration,
+ final Configuration clusterConfiguration,
+ final boolean isCheckpointingEnabled) throws Exception {
+
+ checkNotNull(jobRestartStrategyConfiguration);
+ checkNotNull(clusterConfiguration);
+
+ final String restartStrategyClassName =
clusterConfiguration.getString(
+
RestartBackoffTimeStrategyOptions.RESTART_BACKOFF_TIME_STRATEGY_CLASS_NAME);
+
+ if (restartStrategyClassName != null) {
+ // create new restart strategy directly if it is
specified in cluster config
+ return
createRestartStrategyFactoryInternal(clusterConfiguration);
+ } else {
+ // adapt the legacy restart strategy configs as new
restart strategy configs
+ final Configuration adaptedConfiguration =
getAdaptedConfiguration(
+ jobRestartStrategyConfiguration,
+ clusterConfiguration,
+ isCheckpointingEnabled);
+
+ // create new restart strategy from the adapted config
+ return
createRestartStrategyFactoryInternal(adaptedConfiguration);
+ }
+ }
+
+ /**
+* Decides the {@link RestartBackoffTimeStrategy} to use and its params
based on legacy configs,
+* and records its class name and the params into a adapted
configuration.
+*
+* The decision making is as follows:
+*
+* Use strategy of {@link
[GitHub] [flink] zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy…
zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime]
Implement new generation restart strategy loader which also respects legacy…
URL: https://github.com/apache/flink/pull/8912#discussion_r323583514
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/failover/flip1/RestartBackoffTimeStrategyFactoryLoader.java
##
@@ -0,0 +1,393 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.executiongraph.failover.flip1;
+
+import org.apache.flink.api.common.restartstrategy.RestartStrategies;
+import org.apache.flink.configuration.AkkaOptions;
+import org.apache.flink.configuration.ConfigConstants;
+import org.apache.flink.configuration.ConfigOption;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.RestartBackoffTimeStrategyOptions;
+import
org.apache.flink.runtime.executiongraph.restart.FixedDelayRestartStrategy;
+import org.apache.flink.runtime.executiongraph.restart.NoRestartStrategy;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.concurrent.TimeUnit;
+
+import scala.concurrent.duration.Duration;
+
+import static org.apache.flink.util.Preconditions.checkArgument;
+import static org.apache.flink.util.Preconditions.checkNotNull;
+
+/**
+ * A utility class to load {@link RestartBackoffTimeStrategy.Factory} from the
configuration.
+ */
+public class RestartBackoffTimeStrategyFactoryLoader {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RestartBackoffTimeStrategyFactoryLoader.class);
+
+ private static final String CREATE_METHOD = "createFactory";
+
+ /**
+* Creates proper {@link RestartBackoffTimeStrategy.Factory}.
+* If new version restart strategy is specified, will directly use it.
+* Otherwise will decide based on legacy restart strategy configs.
+*
+* @param jobRestartStrategyConfiguration restart configuration given
within the job graph
+* @param clusterConfiguration cluster(server-side) configuration,
usually represented as jobmanager config
+* @param isCheckpointingEnabled if checkpointing was enabled for the
job
+* @return new version restart strategy factory
+*/
+ public static RestartBackoffTimeStrategy.Factory
createRestartStrategyFactory(
+ final RestartStrategies.RestartStrategyConfiguration
jobRestartStrategyConfiguration,
+ final Configuration clusterConfiguration,
+ final boolean isCheckpointingEnabled) throws Exception {
+
+ checkNotNull(jobRestartStrategyConfiguration);
+ checkNotNull(clusterConfiguration);
+
+ final String restartStrategyClassName =
clusterConfiguration.getString(
+
RestartBackoffTimeStrategyOptions.RESTART_BACKOFF_TIME_STRATEGY_CLASS_NAME);
+
+ if (restartStrategyClassName != null) {
+ // create new restart strategy directly if it is
specified in cluster config
+ return
createRestartStrategyFactoryInternal(clusterConfiguration);
+ } else {
+ // adapt the legacy restart strategy configs as new
restart strategy configs
+ final Configuration adaptedConfiguration =
getAdaptedConfiguration(
+ jobRestartStrategyConfiguration,
+ clusterConfiguration,
+ isCheckpointingEnabled);
+
+ // create new restart strategy from the adapted config
+ return
createRestartStrategyFactoryInternal(adaptedConfiguration);
+ }
+ }
+
+ /**
+* Decides the {@link RestartBackoffTimeStrategy} to use and its params
based on legacy configs,
+* and records its class name and the params into a adapted
configuration.
+*
+* The decision making is as follows:
+*
+* Use strategy of {@link
[GitHub] [flink] zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime] Implement new generation restart strategy loader which also respects legacy…
zhuzhurk commented on a change in pull request #8912: [FLINK-12709] [runtime]
Implement new generation restart strategy loader which also respects legacy…
URL: https://github.com/apache/flink/pull/8912#discussion_r323583682
##
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/failover/flip1/RestartBackoffTimeStrategyFactoryLoader.java
##
@@ -0,0 +1,393 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.executiongraph.failover.flip1;
+
+import org.apache.flink.api.common.restartstrategy.RestartStrategies;
+import org.apache.flink.configuration.AkkaOptions;
+import org.apache.flink.configuration.ConfigConstants;
+import org.apache.flink.configuration.ConfigOption;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.RestartBackoffTimeStrategyOptions;
+import
org.apache.flink.runtime.executiongraph.restart.FixedDelayRestartStrategy;
+import org.apache.flink.runtime.executiongraph.restart.NoRestartStrategy;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.concurrent.TimeUnit;
+
+import scala.concurrent.duration.Duration;
+
+import static org.apache.flink.util.Preconditions.checkArgument;
+import static org.apache.flink.util.Preconditions.checkNotNull;
+
+/**
+ * A utility class to load {@link RestartBackoffTimeStrategy.Factory} from the
configuration.
+ */
+public class RestartBackoffTimeStrategyFactoryLoader {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RestartBackoffTimeStrategyFactoryLoader.class);
+
+ private static final String CREATE_METHOD = "createFactory";
+
+ /**
+* Creates proper {@link RestartBackoffTimeStrategy.Factory}.
+* If new version restart strategy is specified, will directly use it.
+* Otherwise will decide based on legacy restart strategy configs.
+*
+* @param jobRestartStrategyConfiguration restart configuration given
within the job graph
+* @param clusterConfiguration cluster(server-side) configuration,
usually represented as jobmanager config
+* @param isCheckpointingEnabled if checkpointing was enabled for the
job
+* @return new version restart strategy factory
+*/
+ public static RestartBackoffTimeStrategy.Factory
createRestartStrategyFactory(
+ final RestartStrategies.RestartStrategyConfiguration
jobRestartStrategyConfiguration,
+ final Configuration clusterConfiguration,
+ final boolean isCheckpointingEnabled) throws Exception {
+
+ checkNotNull(jobRestartStrategyConfiguration);
+ checkNotNull(clusterConfiguration);
+
+ final String restartStrategyClassName =
clusterConfiguration.getString(
+
RestartBackoffTimeStrategyOptions.RESTART_BACKOFF_TIME_STRATEGY_CLASS_NAME);
+
+ if (restartStrategyClassName != null) {
+ // create new restart strategy directly if it is
specified in cluster config
+ return
createRestartStrategyFactoryInternal(clusterConfiguration);
+ } else {
+ // adapt the legacy restart strategy configs as new
restart strategy configs
+ final Configuration adaptedConfiguration =
getAdaptedConfiguration(
+ jobRestartStrategyConfiguration,
+ clusterConfiguration,
+ isCheckpointingEnabled);
+
+ // create new restart strategy from the adapted config
+ return
createRestartStrategyFactoryInternal(adaptedConfiguration);
+ }
+ }
+
+ /**
+* Decides the {@link RestartBackoffTimeStrategy} to use and its params
based on legacy configs,
+* and records its class name and the params into a adapted
configuration.
+*
+* The decision making is as follows:
+*
+* Use strategy of {@link
[jira] [Commented] (FLINK-14053) blink planner dense_rank corner case bug
[
https://issues.apache.org/jira/browse/FLINK-14053?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16928289#comment-16928289
]
Jingsong Lee commented on FLINK-14053:
--
[~jackylau] Thanks for reporting this bug. Yes, DenseRankAggFunction should be
same as RankAggFunction, you can do some abstract to RankLikeAggFunctionBase,
these two functions can share some logical. Feel free to submit a PR, I can
review it.
> blink planner dense_rank corner case bug
>
>
> Key: FLINK-14053
> URL: https://issues.apache.org/jira/browse/FLINK-14053
> Project: Flink
> Issue Type: Bug
> Components: Table SQL / Planner
>Affects Versions: 1.9.0
>Reporter: jackylau
>Priority: Major
> Fix For: 1.10.0
>
>
> sql :
> val rank =
> """
> |SELECT
> | gradeId,
> | classId,
> | stuId,
> | score,
> | dense_rank() OVER (PARTITION BY gradeId, classId ORDER BY score asc) as
> dense_rank_num
> |FROM student
> |
> """.stripMargin
> sample date:
> row("grade2", "class2", "0006", 90),
> row("grade1", "class2", "0007", 90),
> row("grade1", "class1", "0001", 95),
> row("grade1", "class1", "0002", 94),
> row("grade1", "class1", "0003", 97),
> row("grade1", "class1", "0004", 95),
> row("grade1", "class1", "0005", 0)
> the dense_rank ranks from 0, but it should be from 1
>
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
[jira] [Updated] (FLINK-14065) Log metric name when the metric fails on registration/unregistration
[
https://issues.apache.org/jira/browse/FLINK-14065?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Zhilong Hong updated FLINK-14065:
-
Description:
When MetricGroup registers Metrics in MetricRegistryImpl, sometimes the
registration fails due to exceptions. However, currently it only logs "{{Error
while registering metric}}" with no more information, which is inconvenient
for users to troubleshoot which metric fails and why it fails.
Also, the warning log in registration and unregistration are both "{{Error
while registering metric}}". This will lead users to confusion (although users
can locate the correct place according to the call stack).
So I propose to log metric name when the metrics fails on
registration/unregistration.
was:
When MetricGroup registers Metrics in MetricRegistryImpl, sometimes the
registration fails due to exceptions. However, currently it only logs {{"Error
while registering metric" }}with no more information, which is inconvenient for
users to troubleshoot which metric fails and why it fails.
Also, the warning log in registration and unregistration are both "{{Error
while registering metric}}". This will lead users to confusion (although users
can locate the correct place according to the call stack).
So I propose to log metric name when the metrics fails on
registration/unregistration.
> Log metric name when the metric fails on registration/unregistration
>
>
> Key: FLINK-14065
> URL: https://issues.apache.org/jira/browse/FLINK-14065
> Project: Flink
> Issue Type: Improvement
> Components: Runtime / Metrics
>Affects Versions: 1.9.0
>Reporter: Zhilong Hong
>Priority: Trivial
> Labels: easyfix
> Fix For: 1.10.0
>
>
> When MetricGroup registers Metrics in MetricRegistryImpl, sometimes the
> registration fails due to exceptions. However, currently it only logs
> "{{Error while registering metric}}" with no more information, which is
> inconvenient for users to troubleshoot which metric fails and why it fails.
> Also, the warning log in registration and unregistration are both "{{Error
> while registering metric}}". This will lead users to confusion (although
> users can locate the correct place according to the call stack).
> So I propose to log metric name when the metrics fails on
> registration/unregistration.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
[GitHub] [flink] flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#issuecomment-530686777 ## CI report: * d15f9751632d4c7897f68a0f8829d5facbdfe14e : PENDING [Build](https://travis-ci.com/flink-ci/flink/builds/127138398) 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[jira] [Comment Edited] (FLINK-14065) Log metric name when the metric fails on registration/unregistration
[
https://issues.apache.org/jira/browse/FLINK-14065?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16928279#comment-16928279
]
Zhilong Hong edited comment on FLINK-14065 at 9/12/19 6:41 AM:
---
It would be my pleasure if I can help to fix this issue.
I'm planning to add metric name in log string in MetricRegistryImpl#register
like this:
{{LOG.warn("Error while registering metric: {}.", metricName, e);}}
and change the log string in MetricRegistryImpl#unregister like this:
{{LOG.warn("Error while unregistering metric: {}.", metricName, e);}}
was (Author: thesharing):
It would be my pleasure if I can help to fix this issue.
I'm planning to add metric name in log string in MetricRegistryImpl#register
like this:
{{LOG.warn("Error while registering metric: {}.", metricName, e);}}
and change the log string in MetricRegistryImpl#unregister like this:
{{LOG.warn("Error while unregistering metric: {}.", metricName, e);}}
> Log metric name when the metric fails on registration/unregistration
>
>
> Key: FLINK-14065
> URL: https://issues.apache.org/jira/browse/FLINK-14065
> Project: Flink
> Issue Type: Improvement
> Components: Runtime / Metrics
>Affects Versions: 1.9.0
>Reporter: Zhilong Hong
>Priority: Trivial
> Labels: easyfix
> Fix For: 1.10.0
>
>
> When MetricGroup registers Metrics in MetricRegistryImpl, sometimes the
> registration fails due to exceptions. However, currently it only logs
> {{"Error while registering metric" }}with no more information, which is
> inconvenient for users to troubleshoot which metric fails and why it fails.
> Also, the warning log in registration and unregistration are both "{{Error
> while registering metric}}". This will lead users to confusion (although
> users can locate the correct place according to the call stack).
> So I propose to log metric name when the metrics fails on
> registration/unregistration.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
[jira] [Commented] (FLINK-14065) Log metric name when the metric fails on registration/unregistration
[
https://issues.apache.org/jira/browse/FLINK-14065?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16928279#comment-16928279
]
Zhilong Hong commented on FLINK-14065:
--
It would be my pleasure if I can help to fix this issue.
I'm planning to add metric name in log string in MetricRegistryImpl#register
like this:
{{LOG.warn("Error while registering metric: {}.", metricName, e);}}
and change the log string in MetricRegistryImpl#unregister like this:
{{LOG.warn("Error while unregistering metric: {}.", metricName, e);}}
> Log metric name when the metric fails on registration/unregistration
>
>
> Key: FLINK-14065
> URL: https://issues.apache.org/jira/browse/FLINK-14065
> Project: Flink
> Issue Type: Improvement
> Components: Runtime / Metrics
>Affects Versions: 1.9.0
>Reporter: Zhilong Hong
>Priority: Trivial
> Labels: easyfix
> Fix For: 1.10.0
>
>
> When MetricGroup registers Metrics in MetricRegistryImpl, sometimes the
> registration fails due to exceptions. However, currently it only logs
> {{"Error while registering metric" }}with no more information, which is
> inconvenient for users to troubleshoot which metric fails and why it fails.
> Also, the warning log in registration and unregistration are both "{{Error
> while registering metric}}". This will lead users to confusion (although
> users can locate the correct place according to the call stack).
> So I propose to log metric name when the metrics fails on
> registration/unregistration.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
[GitHub] [flink] flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#issuecomment-530682070 Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community to review your pull request. We will use this comment to track the progress of the review. ## Automated Checks Last check on commit d15f9751632d4c7897f68a0f8829d5facbdfe14e (Thu Sep 12 06:40:05 UTC 2019) **Warnings:** * **2 pom.xml files were touched**: Check for build and licensing issues. * No documentation files were touched! Remember to keep the Flink docs up to date! Mention the bot in a comment to re-run the automated checks. ## Review Progress * ❓ 1. The [description] looks good. * ❓ 2. There is [consensus] that the contribution should go into to Flink. * ❓ 3. Needs [attention] from. * ❓ 4. The change fits into the overall [architecture]. * ❓ 5. Overall code [quality] is good. Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands The @flinkbot bot supports the following commands: - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`) - `@flinkbot approve all` to approve all aspects - `@flinkbot approve-until architecture` to approve everything until `architecture` - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention - `@flinkbot disapprove architecture` to remove an approval you gave earlier 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime]
Facilitate enabling new scheduler in MiniCluster Tests
URL: https://github.com/apache/flink/pull/9675#discussion_r323576390
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/category/AlsoRunWithSchedulerNG.java
##
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.testutils.junit.category;
+
+/**
+ * Category marker interface to run tests with SchedulerNG.
+ */
+public interface AlsoRunWithSchedulerNG {
Review comment:
Have verified the annotation manually with a temporary patch. The annotated
test can be found out and executed with new scheduler, although it hangs in the
scheduling process.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#issuecomment-530682070 Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community to review your pull request. We will use this comment to track the progress of the review. ## Automated Checks Last check on commit d15f9751632d4c7897f68a0f8829d5facbdfe14e (Thu Sep 12 06:38:02 UTC 2019) **Warnings:** * **2 pom.xml files were touched**: Check for build and licensing issues. * No documentation files were touched! Remember to keep the Flink docs up to date! Mention the bot in a comment to re-run the automated checks. ## Review Progress * ❓ 1. The [description] looks good. * ❓ 2. There is [consensus] that the contribution should go into to Flink. * ❓ 3. Needs [attention] from. * ❓ 4. The change fits into the overall [architecture]. * ❓ 5. Overall code [quality] is good. Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands The @flinkbot bot supports the following commands: - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`) - `@flinkbot approve all` to approve all aspects - `@flinkbot approve-until architecture` to approve everything until `architecture` - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention - `@flinkbot disapprove architecture` to remove an approval you gave earlier 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[jira] [Created] (FLINK-14065) Log metric name when the metric fails on registration/unregistration
Zhilong Hong created FLINK-14065:
Summary: Log metric name when the metric fails on
registration/unregistration
Key: FLINK-14065
URL: https://issues.apache.org/jira/browse/FLINK-14065
Project: Flink
Issue Type: Improvement
Components: Runtime / Metrics
Affects Versions: 1.9.0
Reporter: Zhilong Hong
Fix For: 1.10.0
When MetricGroup registers Metrics in MetricRegistryImpl, sometimes the
registration fails due to exceptions. However, currently it only logs {{"Error
while registering metric" }}with no more information, which is inconvenient for
users to troubleshoot which metric fails and why it fails.
Also, the warning log in registration and unregistration are both "{{Error
while registering metric}}". This will lead users to confusion (although users
can locate the correct place according to the call stack).
So I propose to log metric name when the metrics fails on
registration/unregistration.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
[GitHub] [flink] zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime]
Facilitate enabling new scheduler in MiniCluster Tests
URL: https://github.com/apache/flink/pull/9675#discussion_r323575794
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/category/AlsoRunWithSchedulerNG.java
##
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.testutils.junit.category;
+
+/**
+ * Category marker interface to run tests with SchedulerNG.
+ */
+public interface AlsoRunWithSchedulerNG {
Review comment:
No test is annotated since the `DefaultScheduler` has not implemented the
`startScheduling` interface yet.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] flinkbot commented on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
flinkbot commented on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#issuecomment-530686777 ## CI report: * d15f9751632d4c7897f68a0f8829d5facbdfe14e : UNKNOWN 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#issuecomment-530682070 Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community to review your pull request. We will use this comment to track the progress of the review. ## Automated Checks Last check on commit d15f9751632d4c7897f68a0f8829d5facbdfe14e (Thu Sep 12 06:26:51 UTC 2019) **Warnings:** * **2 pom.xml files were touched**: Check for build and licensing issues. * No documentation files were touched! Remember to keep the Flink docs up to date! Mention the bot in a comment to re-run the automated checks. ## Review Progress * ❓ 1. The [description] looks good. * ❓ 2. There is [consensus] that the contribution should go into to Flink. * ❓ 3. Needs [attention] from. * ❓ 4. The change fits into the overall [architecture]. * ❓ 5. Overall code [quality] is good. Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands The @flinkbot bot supports the following commands: - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`) - `@flinkbot approve all` to approve all aspects - `@flinkbot approve-until architecture` to approve everything until `architecture` - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention - `@flinkbot disapprove architecture` to remove an approval you gave earlier 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#discussion_r323573156 ## File path: flink-connectors/flink-connector-elasticsearch6/pom.xml ## @@ -195,7 +195,6 @@ under the License. org.apache.maven.plugins maven-surefire-plugin - 2.12.2 Review comment: Without this fix, the testing for this module fails when `groups` is specified in maven building. The error is as below: > initializationError(org.junit.runner.manipulation.Filter) Time elapsed: 0.002 sec <<< ERROR! > java.lang.Exception: No tests found matching *org.apache.flink.testutils.junit.category.AlsoRunWithSchedulerNG from org.junit.runner.Request$1@7e0ea639 >at org.junit.internal.requests.FilterRequest.getRunner(FilterRequest.java:40) >at org.junit.runner.JUnitCore.run(JUnitCore.java:115) >at org.apache.maven.surefire.junitcore.JUnitCoreWrapper.execute(JUnitCoreWrapper.java:62) >at org.apache.maven.surefire.junitcore.JUnitCoreProvider.invoke(JUnitCoreProvider.java:139) >at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) >at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) >at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >at java.lang.reflect.Method.invoke(Method.java:498) >at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:189) >at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:165) >at org.apache.maven.surefire.booter.ProviderFactory.invokeProvider(ProviderFactory.java:85) >at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:113) >at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:75) 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
flinkbot edited a comment on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#issuecomment-530682070 Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community to review your pull request. We will use this comment to track the progress of the review. ## Automated Checks Last check on commit d15f9751632d4c7897f68a0f8829d5facbdfe14e (Thu Sep 12 06:24:48 UTC 2019) **Warnings:** * **2 pom.xml files were touched**: Check for build and licensing issues. * No documentation files were touched! Remember to keep the Flink docs up to date! Mention the bot in a comment to re-run the automated checks. ## Review Progress * ❓ 1. The [description] looks good. * ❓ 2. There is [consensus] that the contribution should go into to Flink. * ❓ 3. Needs [attention] from. * ❓ 4. The change fits into the overall [architecture]. * ❓ 5. Overall code [quality] is good. Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands The @flinkbot bot supports the following commands: - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`) - `@flinkbot approve all` to approve all aspects - `@flinkbot approve-until architecture` to approve everything until `architecture` - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention - `@flinkbot disapprove architecture` to remove an approval you gave earlier 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#discussion_r323572659 ## File path: pom.xml ## @@ -637,6 +639,18 @@ under the License. + Review comment: This profile is not necessary but simplifies the way to run tests for annotated MiniClusters again the new scheduler. We can run "mvn verify -Dscheduler.type=ng -Dtest.groups=org.apache.flink.testutils.junit.category.AlsoRunWithSchedulerNG" instead. To find out whether all the MiniCluster tests(even not annotated with `AlsoRunWithSchedulerNG`) can pass against the new scheduler, we can run "mvn verify -Dscheduler.type=ng". 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
zhuzhurk commented on a change in pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#discussion_r323572659 ## File path: pom.xml ## @@ -637,6 +639,18 @@ under the License. + Review comment: This profile is not necessary but simplifies the way to run tests for annotated MiniClusters again the new scheduler. We can run "mvn verify -Dscheduler.type=ng -Dtest.groups=org.apache.flink.testutils.junit.category.AlsoRunWithSchedulerNG" instead. And to find out whether all the MiniCluster tests(even not annotated with `AlsoRunWithSchedulerNG`) can pass against the new scheduler, we can run "mvn verify -Dscheduler.type=ng". 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[jira] [Commented] (FLINK-14054) Enable checkpointing via job configuration
[
https://issues.apache.org/jira/browse/FLINK-14054?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16928259#comment-16928259
]
TisonKun commented on FLINK-14054:
--
I might mistake that we are already able to configure parallelism and other
options in job level. [~qinjunjerry] do you have an idea how the config key of
the configuration you proposed should be?
> Enable checkpointing via job configuration
> --
>
> Key: FLINK-14054
> URL: https://issues.apache.org/jira/browse/FLINK-14054
> Project: Flink
> Issue Type: Improvement
> Components: Runtime / Configuration
>Reporter: Jun Qin
>Priority: Major
>
> Currently enabling checkpointing can only be done via the job code, see the
> following quote from this Flink
> [checkpointing|https://ci.apache.org/projects/flink/flink-docs-release-1.9/dev/stream/state/checkpointing.html#enabling-and-configuring-checkpointing]
> doc:
> {quote}By default, checkpointing is disabled. To enable checkpointing, call
> {{enableCheckpointing(n)}} on the {{StreamExecutionEnvironment}}, where _n_
> is the checkpoint interval in milliseconds.
> {quote}
> This makes enabling checkingpointing after the job code has been released
> difficult: one has to change and rebuild the job code.
> In addition, not only for developer, making checkpointing enabling
> configurable is also of interest for operation teams:
> * They may want to enable checkpointing for production but disable in test
> (e.g., to save storage space)
> * They may want to try out with and without checkpointing to evaluate the
> impact to the job behaviour and performance.
> Therefore, this request. Thanks.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
[GitHub] [flink] flinkbot commented on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
flinkbot commented on issue #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests URL: https://github.com/apache/flink/pull/9675#issuecomment-530682070 Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community to review your pull request. We will use this comment to track the progress of the review. ## Automated Checks Last check on commit d15f9751632d4c7897f68a0f8829d5facbdfe14e (Thu Sep 12 06:16:41 UTC 2019) **Warnings:** * **2 pom.xml files were touched**: Check for build and licensing issues. * No documentation files were touched! Remember to keep the Flink docs up to date! Mention the bot in a comment to re-run the automated checks. ## Review Progress * ❓ 1. The [description] looks good. * ❓ 2. There is [consensus] that the contribution should go into to Flink. * ❓ 3. Needs [attention] from. * ❓ 4. The change fits into the overall [architecture]. * ❓ 5. Overall code [quality] is good. Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands The @flinkbot bot supports the following commands: - `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`) - `@flinkbot approve all` to approve all aspects - `@flinkbot approve-until architecture` to approve everything until `architecture` - `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention - `@flinkbot disapprove architecture` to remove an approval you gave earlier 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] zhuzhurk opened a new pull request #9675: [FLINK-13953] [runtime] Facilitate enabling new scheduler in MiniCluster Tests
zhuzhurk opened a new pull request #9675: [FLINK-13953] [runtime] Facilitate
enabling new scheduler in MiniCluster Tests
URL: https://github.com/apache/flink/pull/9675
## What is the purpose of the change
Currently, tests using the {{MiniCluster}} use the legacy scheduler by
default. Once the new scheduler is implemented, we should run tests with the
new scheduler enabled. However, it is not expected that all tests will pass
immediately. Therefore, it should be possible to enable the new scheduler for a
subset of tests.
In the first step the tests should be able to run manually against new
scheduler.
*Acceptance Criteria*
* A junit test category {{AlsoRunWithSchedulerNG}} can be used to mark
MiniCluster tests.
* A new maven profile {{scheduler-ng}} will be enabled to support running
{{AlsoRunWithSchedulerNG}} annotated tests with the new scheduler.
## Brief change log
The change is inspired by the prototype from @tillrohrmann at
https://github.com/tillrohrmann/flink/commit/dee9e7d82f65fa1697489de743e84dcf02d76711.
- *Added a category marker for new scheduler testing*
- *Enabled setting scheduler type in MiniClusterConfiguration via
environment variable*
- *Added a maven profile to run annotated MiniCluster tests for the new
scheduler*
- *other hotfixes, details see each hotfix commit*
## Verifying this change
This change added tests and can be verified as follows:
- *Added UTs for MiniClusterConfiguration changes*
- *Manually verified the change by running command "mvn verify
-Dscheduler-ng". Verified that only the tests annotated with
`AlsoRunWithSchedulerNG` are executed in this way. Verified that the scheduler
type can be specified by setting maven profiles or properties.*
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no**
/ don't know)
- Anything that affects deployment or recovery: JobManager (and its
components), Checkpointing, Yarn/Mesos, ZooKeeper: (**yes** / no / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (**not applicable** / docs /
JavaDocs / not documented)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[jira] [Updated] (FLINK-13953) Facilitate enabling new Scheduler in MiniCluster Tests
[
https://issues.apache.org/jira/browse/FLINK-13953?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated FLINK-13953:
---
Labels: pull-request-available (was: )
> Facilitate enabling new Scheduler in MiniCluster Tests
> --
>
> Key: FLINK-13953
> URL: https://issues.apache.org/jira/browse/FLINK-13953
> Project: Flink
> Issue Type: Sub-task
> Components: Runtime / Coordination, Tests
>Reporter: Gary Yao
>Assignee: Zhu Zhu
>Priority: Major
> Labels: pull-request-available
>
> Currently, tests using the {{MiniCluster}} use the legacy scheduler by
> default. Once the new scheduler is implemented, we should run tests with the
> new scheduler enabled. However, it is not expected that all tests will pass
> immediately. Therefore, it should be possible to enable the new scheduler for
> a subset of tests.
> In the first step the tests should be able to run manually against new
> scheduler.
> *Acceptance Criteria*
> * A junit test category {{AlsoRunWithSchedulerNG}} can be used to mark
> MiniCluster tests.
> * A new maven profile {{scheduler-ng}} will be enabled to support running
> {{AlsoRunWithSchedulerNG}} annotated tests with the new scheduler.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
[jira] [Commented] (FLINK-14054) Enable checkpointing via job configuration
[
https://issues.apache.org/jira/browse/FLINK-14054?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16928235#comment-16928235
]
TisonKun commented on FLINK-14054:
--
Hi [~qinjunjerry]! Thanks for reporting your requirement. I agree that in your
specific case enable checkpointing via configuration file instead of
configuring inline provides more flexibility.
However, it seems we don't have an aspect setting job configuration in file
yet. Maybe extra efforts are required for introducing such an aspect properly
first.
CC [~twalthr] & [~till.rohrmann] as you are working on configuration recently.
> Enable checkpointing via job configuration
> --
>
> Key: FLINK-14054
> URL: https://issues.apache.org/jira/browse/FLINK-14054
> Project: Flink
> Issue Type: Improvement
> Components: Runtime / Configuration
>Reporter: Jun Qin
>Priority: Major
>
> Currently enabling checkpointing can only be done via the job code, see the
> following quote from this Flink
> [checkpointing|https://ci.apache.org/projects/flink/flink-docs-release-1.9/dev/stream/state/checkpointing.html#enabling-and-configuring-checkpointing]
> doc:
> {quote}By default, checkpointing is disabled. To enable checkpointing, call
> {{enableCheckpointing(n)}} on the {{StreamExecutionEnvironment}}, where _n_
> is the checkpoint interval in milliseconds.
> {quote}
> This makes enabling checkingpointing after the job code has been released
> difficult: one has to change and rebuild the job code.
> In addition, not only for developer, making checkpointing enabling
> configurable is also of interest for operation teams:
> * They may want to enable checkpointing for production but disable in test
> (e.g., to save storage space)
> * They may want to try out with and without checkpointing to evaluate the
> impact to the job behaviour and performance.
> Therefore, this request. Thanks.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
