[FLINK-9028][flip6] perform parameters checking before starting cluster #5726

sihuazhou · 2018-03-20T09:06:37Z

What is the purpose of the change

Perform parameters checking before starting cluster to prevent to setup a problematic cluster.

Brief change log

perform parameters checking before starting cluster

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

no

sihuazhou · 2018-03-20T09:07:18Z

CC: @tillrohrmann

tillrohrmann

Thanks for the contribution @sihuazhou. The fix is a good idea. I think we can still improve it a bit by refactoring the verification logic and moving the check to AbstractYarnClusterDescriptor#deployInternal.

tillrohrmann · 2018-03-20T09:37:34Z

flink-yarn/src/main/java/org/apache/flink/yarn/cli/FlinkYarnSessionCli.java

+		// aim to check the parameters
+		ContaineredTaskManagerParameters.create(
+			effectiveConfiguration,
+			effectiveConfiguration.getInteger(TaskManagerOptions.TASK_MANAGER_HEAP_MEMORY), 1);


I think we should pull the verification logic out of the ContaineredTaskManagerParameters#create method and place it somewhere else. I would also suggest to move this check to AbstractYarnClusterDescriptor#deployInternal and use the provided ClusterSpecification#getTaskManagerMemory for the memory.

👍 I will pull it out and add a test to guard it.

sihuazhou · 2018-03-20T11:06:12Z

@tillrohrmann I made a fixup according to your comments, could you please have a look again?

tillrohrmann

I had some more comments.

tillrohrmann · 2018-03-20T12:44:23Z

...c/test/java/org/apache/flink/runtime/resourcemanager/ResourceManagerRuntimeServicesTest.java

+/**
+ * Tests to guard {@link ResourceManagerRuntimeServices}.
+ */
+public class ResourceManagerRuntimeServicesTest {


missing extends TestLogger

tillrohrmann · 2018-03-20T12:45:06Z

flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java

@@ -423,6 +431,9 @@ public void terminateCluster(ApplicationId applicationId) throws FlinkException
 			@Nullable JobGraph jobGraph,
 			boolean detached) throws Exception {

+		// ------------------ Check if configuration is valid --------------------
+		checkConfig(clusterSpecification);


let's not abbreviate and name it validateClusterSpecification

tillrohrmann · 2018-03-20T12:50:11Z

flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java

@@ -409,6 +411,12 @@ public void terminateCluster(ApplicationId applicationId) throws FlinkException
 		}
 	}

+	private void checkConfig(ClusterSpecification clusterSpecification) {


This should throw a checked exception. Something like InvalidConfigurationException or so. But not just fail with an IllegalStateException.

tillrohrmann · 2018-03-20T12:52:00Z

flink-yarn/src/main/java/org/apache/flink/yarn/AbstractYarnClusterDescriptor.java

+	private void checkConfig(ClusterSpecification clusterSpecification) {
+		long taskManagerMemorySize = clusterSpecification.getTaskManagerMemoryMB();
+		long cutoff = ResourceManagerRuntimeServices.calculateCutoffMB(flinkConfiguration, taskManagerMemorySize);
+		TaskManagerServices.calculateHeapSizeMB(taskManagerMemorySize - cutoff, flinkConfiguration);


I somehow don't like that we calculate some value just in order to check whether an unchecked exception is thrown. This should be imo more explicit. Or at least it should get a comment explaining what's going on here.

Hmm...This is a bit tricky, the calculation logic is a bit hard to be separated from the check logical so clearly, because when we perform the checking we also need to do the calculation by the way. So pull the check logical from the calculateFunction to be a separated function means we will have some
code duplication. So I'd like to add a comment here, if you still think we should pull out a new function I would just do it ;).

tillrohrmann · 2018-03-20T12:53:54Z

...rc/main/java/org/apache/flink/runtime/clusterframework/ContaineredTaskManagerParameters.java

-
-		final long javaMemorySizeMB = containerMemoryMB - cutoff;
+		// (1) try to compute how much memory used by container
+		final long cutoffMB = ResourceManagerRuntimeServices.calculateCutoffMB(config, containerMemoryMB);


I would keep this method in the ContaineredTaskManagerParameters class.

sihuazhou · 2018-03-20T14:13:35Z

@tillrohrmann I have addressed your comments, could you please review it again.

tillrohrmann · 2018-03-20T14:26:11Z

Thanks for your contribution @sihuazhou. Changes look good to me. Merging this PR once Travis gave green light.

…cluster This closes apache#5726.

sihuazhou · 2018-03-20T15:08:59Z

...found a checkstyle bug in my local built, I made a fixup.

tillrohrmann · 2018-03-20T15:19:27Z

Have seen it as well and fixed it. So no worries.

…cluster This closes apache#5726.

…cluster This closes #5726.

…cluster This closes apache#5726.

perform parameters checking before starting cluster.

f65351b

sihuazhou changed the title ~~[FLINK-FLINK-9028][flip6] perform parameters checking before starting cluster~~ [FLINK-9028][flip6] perform parameters checking before starting cluster Mar 20, 2018

tillrohrmann requested changes Mar 20, 2018

View reviewed changes

sihuazhou force-pushed the flip6_check_parameter_before_start_cluster branch from efd1ec5 to b39bd8b Compare March 20, 2018 11:39

Till's comments.

c984370

sihuazhou force-pushed the flip6_check_parameter_before_start_cluster branch from b39bd8b to c984370 Compare March 20, 2018 11:42

tillrohrmann requested changes Mar 20, 2018

View reviewed changes

sihuazhou force-pushed the flip6_check_parameter_before_start_cluster branch from b8776f2 to 2e65190 Compare March 20, 2018 14:10

Till's comments.

f6e1105

sihuazhou force-pushed the flip6_check_parameter_before_start_cluster branch from 2e65190 to f6e1105 Compare March 20, 2018 14:40

tillrohrmann pushed a commit to tillrohrmann/flink that referenced this pull request Mar 20, 2018

[FLINK-9028] [yarn] Perform parameters checking before Yarn starting …

b6654dc

…cluster This closes apache#5726.

tillrohrmann pushed a commit to tillrohrmann/flink that referenced this pull request Mar 21, 2018

[FLINK-9028] [yarn] Perform parameters checking before Yarn starting …

d60f225

…cluster This closes apache#5726.

asfgit closed this in 38aa863 Mar 21, 2018

asfgit pushed a commit that referenced this pull request Mar 21, 2018

[FLINK-9028] [yarn] Perform parameters checking before Yarn starting …

fecc190

…cluster This closes #5726.

sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018

[FLINK-9028] [yarn] Perform parameters checking before Yarn starting …

f35c42c

…cluster This closes apache#5726.

rmetzger added the component=Runtime/Coordination label Mar 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-9028][flip6] perform parameters checking before starting cluster #5726

[FLINK-9028][flip6] perform parameters checking before starting cluster #5726

sihuazhou commented Mar 20, 2018

sihuazhou commented Mar 20, 2018

tillrohrmann left a comment

tillrohrmann Mar 20, 2018

sihuazhou Mar 20, 2018

sihuazhou commented Mar 20, 2018

tillrohrmann left a comment

tillrohrmann Mar 20, 2018

tillrohrmann Mar 20, 2018

tillrohrmann Mar 20, 2018

tillrohrmann Mar 20, 2018

sihuazhou Mar 20, 2018

tillrohrmann Mar 20, 2018

sihuazhou commented Mar 20, 2018

tillrohrmann commented Mar 20, 2018

sihuazhou commented Mar 20, 2018

tillrohrmann commented Mar 20, 2018

[FLINK-9028][flip6] perform parameters checking before starting cluster #5726

[FLINK-9028][flip6] perform parameters checking before starting cluster #5726

Conversation

sihuazhou commented Mar 20, 2018

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

sihuazhou commented Mar 20, 2018

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihuazhou commented Mar 20, 2018

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihuazhou commented Mar 20, 2018

tillrohrmann commented Mar 20, 2018

sihuazhou commented Mar 20, 2018

tillrohrmann commented Mar 20, 2018