[FLINK-15437][yarn] Apply dynamic properties early on client side. #10728

xintongsong · 2019-12-31T05:16:07Z

What is the purpose of the change

This PR applies dynamic properties early on client side, to make sure the client uses the correct configurations set via dynamic properties.

Brief change log

475b642: Write yarn properties file without YarnClusterDescriptor.
322a659: Apply dynamic properties early on client side.

Verifying this change

Updated FlinkYarnSessionCliTest#testDynamicProperties

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

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

xintongsong · 2019-12-31T05:18:00Z

cc @wangyang0918 @tisonkun @kl0u

flinkbot · 2019-12-31T05:18:14Z

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 322a659 (Tue Dec 31 05:18:13 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 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

flinkbot · 2019-12-31T06:07:54Z

CI report:

4bae531 Travis: SUCCESS Azure: SUCCESS
4cd98f1 Travis: SUCCESS Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

wangyang0918

@xintongsong Thanks for your contribution. I think this PR is a good fix. We apply the dynamic properties earlier in the client and then ship the updated flink config to jobmanager. So we will not need to apply the dynamic properties again in the jobmanager. And i do not find out some negative effects.

For the magic yarn properties file, we keep the same behavior now and try to remove it in the next major release.

wangyang0918 · 2020-01-02T09:49:21Z

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

@@ -1045,10 +1031,6 @@ private ApplicationReport startAppMaster(
 			appMasterEnv.put(YarnConfigKeys.ENV_KRB5_PATH, remoteKrb5Path.toString());
 		}

-		if (dynamicPropertiesEncoded != null) {


Since we have apply all the dynamic properties to the client uploaded flink configuration, so we will not need to set the ENV_DYNAMIC_PROPERTIES and parse it in the jobmanager. So YarnConfigKeys.ENV_DYNAMIC_PROPERTIES could be removed and all the usage on the jobmanager side should also be removed.

wangyang0918 · 2020-01-02T09:56:42Z

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

@@ -369,6 +372,14 @@ public Configuration applyCommandLineOptionsToConfiguration(CommandLine commandL
 			effectiveConfiguration.setInteger(TaskManagerOptions.NUM_TASK_SLOTS, Integer.parseInt(commandLine.getOptionValue(slots.getOpt())));
 		}

+		dynamicPropertiesEncoded = encodeDynamicProperties(commandLine);
+		if (dynamicPropertiesEncoded != null && !dynamicPropertiesEncoded.isEmpty()) {


I think the null check is unnecessary since the encodeDynamicProperties will always return a NonNull value.

…ointUtils#installSecurityContext.

…loadConfiguration.

xintongsong · 2020-01-02T11:30:29Z

@wangyang0918 Thanks for the review, comments addressed.

wangyang0918

@xintongsong Thanks for addressing my comment. LGTM now.

tisonkun

LGTM. Merging...

xintongsong · 2020-01-03T03:06:48Z

Thanks for the review, @tisonkun.

This closes #10728 .

[hotfix][yarn] Write yarn properties file without YarnClusterDescriptor.

475b642

rmetzger added the review=description? label Dec 31, 2019

xintongsong force-pushed the FLINK-15437-D-tm-mem branch from 322a659 to 4bae531 Compare December 31, 2019 05:19

rmetzger added component=CommandLineClient component=Deployment/YARN labels Dec 31, 2019

wangyang0918 requested changes Jan 2, 2020

View reviewed changes

xintongsong added 3 commits January 2, 2020 19:29

[hotfix][yarn] Remove unused argument workingDirectory for YarnEntryp…

cb83a6d

…ointUtils#installSecurityContext.

[hotfix][yarn] Remove unused argument logger for YarnEntrypointUtils#…

5579fd9

…loadConfiguration.

[FLINK-15437][yarn] Apply dynamic properties early on client side.

4cd98f1

xintongsong force-pushed the FLINK-15437-D-tm-mem branch from 4bae531 to 4cd98f1 Compare January 2, 2020 11:30

wangyang0918 approved these changes Jan 2, 2020

View reviewed changes

tisonkun approved these changes Jan 3, 2020

View reviewed changes

tisonkun closed this in 367765b Jan 3, 2020

tisonkun pushed a commit that referenced this pull request Jan 3, 2020

[FLINK-15437][yarn] Apply dynamic properties early on client side.

0a89b9a

This closes #10728 .

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-15437][yarn] Apply dynamic properties early on client side. #10728

[FLINK-15437][yarn] Apply dynamic properties early on client side. #10728

xintongsong commented Dec 31, 2019

xintongsong commented Dec 31, 2019

flinkbot commented Dec 31, 2019

flinkbot commented Dec 31, 2019 •

edited

wangyang0918 left a comment •

edited

wangyang0918 Jan 2, 2020

wangyang0918 Jan 2, 2020

xintongsong commented Jan 2, 2020

wangyang0918 left a comment

tisonkun left a comment

xintongsong commented Jan 3, 2020

[FLINK-15437][yarn] Apply dynamic properties early on client side. #10728

[FLINK-15437][yarn] Apply dynamic properties early on client side. #10728

Conversation

xintongsong commented Dec 31, 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

xintongsong commented Dec 31, 2019

flinkbot commented Dec 31, 2019

Automated Checks

Review Progress

flinkbot commented Dec 31, 2019 • edited

CI report:

wangyang0918 left a comment • edited

Choose a reason for hiding this comment

wangyang0918 Jan 2, 2020

Choose a reason for hiding this comment

wangyang0918 Jan 2, 2020

Choose a reason for hiding this comment

xintongsong commented Jan 2, 2020

wangyang0918 left a comment

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

xintongsong commented Jan 3, 2020

flinkbot commented Dec 31, 2019 •

edited

wangyang0918 left a comment •

edited