[FLINK-12152] Make the vcore that Application Master used configurable for Flink on YARN #8438

yanghua · 2019-05-14T11:33:33Z

What is the purpose of the change

This pull request makes the vcore that Application Master used configurable for Flink on YARN

Brief change log

Add a config option vc for Flink on YARN

Verifying this change

This change is already covered by existing tests, such as testCommandLineClusterSpecification.

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)

flinkbot · 2019-05-14T11:34:42Z

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.

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 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

yanghua · 2019-05-14T12:20:59Z

@tillrohrmann The short config option name may need to be discussed. Currently, I used vc(vcore), it can also be ac(application master vcore) or jc(jobmanager vcore). What's your opinion?

yanghua · 2019-05-22T12:05:17Z

@tillrohrmann Do you have any suggestions about the abbreviations of the parameters?

tillrohrmann · 2019-05-23T16:18:08Z

What about only exposing this feature via the flink-conf.yaml? We could name the configuration parameter yarn.appmaster.vcores which is similar to yarn.containers.vcores which can be used to configure the vcores for the containers.

yanghua · 2019-05-24T02:29:04Z

Actually, IMO, we should not expose this config option via the flink-conf.yaml. Because it's not a general purpose config option, it's used for YARN( a special cluster deploy mode). So I think to expose it as a flink-on-YARN config option make sense. WDYT?

tillrohrmann · 2019-05-24T07:06:05Z

We already expose some deployment specific configuration options via the flink-conf.yaml. See https://ci.apache.org/projects/flink/flink-docs-stable/ops/config.html#yarn for example. Thus, this is ok to do.

yanghua · 2019-05-24T07:12:46Z

@tillrohrmann It seems you are right, we did not use this mode so I have not paid attention. Sorry.

yanghua · 2019-05-24T08:07:04Z

@tillrohrmann What about the new changes?

tillrohrmann

Thanks for opening this PR @yanghua. The changes look good. I had one suggestion concerning the test of the configured value. Moreover, I think we don't need the test if we say that we don't check for the lower bound of vcores.

tillrohrmann · 2019-05-24T16:07:36Z

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

@@ -229,6 +229,13 @@ private void isReadyForDeployment(ClusterSpecification clusterSpecification) thr
 			throw new YarnDeploymentException("Flink configuration object has not been set");
 		}

+		int configuredAmVcores = flinkConfiguration.getInteger(YarnConfigOptions.APP_MASTER_VCORES);
+		if (configuredAmVcores < YarnConfigOptions.APP_MASTER_VCORES.defaultValue()) {


I think we should compare configuredAmVcores against numYarnMaxVcores which we calculate a bit further down. I think we don't have to check that we request fewer cores than 1 because it won't take any effect.

tillrohrmann · 2019-05-24T16:08:33Z

flink-yarn/src/main/java/org/apache/flink/yarn/configuration/YarnConfigOptions.java

+	public static final ConfigOption<Integer> APP_MASTER_VCORES =
+		key("yarn.appmaster.vcores")
+		.defaultValue(1)
+		.withDescription("The number of virtual cores (vcores) used by YARN application master.");


I guess we need to regenerate the configuration documentation because we added a new config option: mvn package -Dgenerate-config-docs -pl flink-docs.

I have tried, but it does not take effect. In addition, it seems all the generated HTML documentation are not tracked by git.

You first need to install flink-yarn. I'll do it while merging.

tillrohrmann · 2019-05-24T16:08:48Z

flink-yarn/src/test/java/org/apache/flink/yarn/YarnClusterDescriptorTest.java

+		} finally {
+			clusterDescriptor.close();
+		}
+	}


I think we don't need this test here.

tillrohrmann

Thanks for the PR @yanghua. LGTM. While merging I'll regenerate the docs.

…e for Flink on YARN This closes apache#8438.

rmetzger added review=description? component=Deployment/YARN labels May 14, 2019

yanghua force-pushed the FLINK-12152 branch from be5a5a1 to 13cefb5 Compare May 24, 2019 08:06

yanghua force-pushed the FLINK-12152 branch from 13cefb5 to 4898d4d Compare May 24, 2019 11:01

tillrohrmann requested changes May 24, 2019

View reviewed changes

yanghua force-pushed the FLINK-12152 branch from 4898d4d to 89efc11 Compare May 25, 2019 02:31

tillrohrmann approved these changes May 25, 2019

View reviewed changes

[FLINK-12152] Make the vcore that Application Master used configurabl…

4f87e68

…e for Flink on YARN This closes apache#8438.

tillrohrmann force-pushed the FLINK-12152 branch from 89efc11 to 4f87e68 Compare May 25, 2019 16:12

tillrohrmann closed this in 4f558e4 May 26, 2019

ashangit pushed a commit to ashangit/flink that referenced this pull request Jun 18, 2019

[FLINK-12152] Make the vcore that Application Master used configurabl…

bb9501c

…e for Flink on YARN This closes apache#8438.

ashangit pushed a commit to criteo-forks/flink that referenced this pull request Jul 9, 2019

[FLINK-12152] Make the vcore that Application Master used configurabl…

c5eccdf

…e for Flink on YARN This closes apache#8438.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-12152] Make the vcore that Application Master used configurable for Flink on YARN #8438

[FLINK-12152] Make the vcore that Application Master used configurable for Flink on YARN #8438

yanghua commented May 14, 2019

flinkbot commented May 14, 2019

yanghua commented May 14, 2019

yanghua commented May 22, 2019

tillrohrmann commented May 23, 2019

yanghua commented May 24, 2019

tillrohrmann commented May 24, 2019

yanghua commented May 24, 2019

yanghua commented May 24, 2019

tillrohrmann left a comment

tillrohrmann May 24, 2019

tillrohrmann May 24, 2019

yanghua May 25, 2019

tillrohrmann May 25, 2019

tillrohrmann May 24, 2019

tillrohrmann left a comment

[FLINK-12152] Make the vcore that Application Master used configurable for Flink on YARN #8438

[FLINK-12152] Make the vcore that Application Master used configurable for Flink on YARN #8438

Conversation

yanghua commented May 14, 2019

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

flinkbot commented May 14, 2019

Review Progress

yanghua commented May 14, 2019

yanghua commented May 22, 2019

tillrohrmann commented May 23, 2019

yanghua commented May 24, 2019

tillrohrmann commented May 24, 2019

yanghua commented May 24, 2019

yanghua commented May 24, 2019

tillrohrmann left a comment

Choose a reason for hiding this comment

tillrohrmann May 24, 2019

Choose a reason for hiding this comment

tillrohrmann May 24, 2019

Choose a reason for hiding this comment

yanghua May 25, 2019

Choose a reason for hiding this comment

tillrohrmann May 25, 2019

Choose a reason for hiding this comment

tillrohrmann May 24, 2019

Choose a reason for hiding this comment

tillrohrmann left a comment

Choose a reason for hiding this comment