[FLINK-11781][yarn] Remove "DISABLED" as possible value for yarn.per-job-cluster.include-user-jar by GJL · Pull Request #7883 · apache/flink

GJL · 2019-03-03T18:17:16Z

What is the purpose of the change

This removes DISABLED as a possible value for the config option yarn.per-job-cluster.include-user-jar. Since Flink 1.5, this feature is broken.

Brief change log

See commits

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): (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-03-03T18:17:57Z

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.
- Approved by @dawidwys [committer]
✅ 2. There is [consensus] that the contribution should go into to Flink.
- Approved by @dawidwys [committer]
❓ 3. Needs [attention] from.
✅ 4. The change fits into the overall [architecture].
- Approved by @dawidwys [committer]
✅ 5. Overall code [quality] is good.
- Approved by @dawidwys [committer]

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

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

zentol · 2019-03-03T22:18:54Z

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


 	/** @see YarnConfigOptions#CLASSPATH_INCLUDE_USER_JAR */
 	public enum UserJarInclusion {
-		DISABLED,


We should check where this is parsed to the enum and throw a meaningful exception explaining that DISABLED has been removed.

From Flink 1.5 to 1.7:

If DISABLED is set, the job keeps failing.

If an invalid value is set, the default value (ORDER) is used, and a warning is logged ("Configuration parameter {} was configured with an invalid value {}. Falling back to default ({}).").

If we would apply the PR as is, we would always log a warning (because DISABLED will be considered an invalid value). You are proposing to throw an exception with a customized message if DISABLED is set. This would help users that are upgrading from 1.4 to 1.8 directly. So I could do that but I think it makes sense to also propagate the exception for all other invalid values instead of logging a warning. What do you think?

+1 for throwing exception rather than just logging, when mismatch.

dawidwys

One thing we should fix is the case sensitivity for Configuration#getAsEnum other than that I had just small suggestions. Overall it looks good.

Personally I like the approach that if a user configures the option, it has to be set correctly (or otherwise an exception will be thrown). This way we won't fallback silently to some other option in case of e.g. spelling mistake. So +1 for current behavior.

dawidwys · 2019-03-05T15:09:43Z

flink-core/src/main/java/org/apache/flink/configuration/Configuration.java

+
+		final String configValue = getString(configOption);
+		try {
+			return T.valueOf(enumClass, configValue);


nit:

Suggested change

return T.valueOf(enumClass, configValue);

return Enum.valueOf(enumClass, configValue);

I think that's better. Fixed it.

dawidwys · 2019-03-05T15:10:24Z

flink-core/src/main/java/org/apache/flink/configuration/Configuration.java

+	 *                                  be parsed as a value of the provided enum class.
+	 */
+	@PublicEvolving
+	public <T extends Enum<T>> T getAsEnum(


nit: This would be more consistent with other getters.

Suggested change

public <T extends Enum<T>> T getAsEnum(

public <T extends Enum<T>> T getEnum(

I think you are right. It differs a bit though in the sense that ConfigOption<Enum> is not possible. On the other hand there is getBytes

I see... then I am ok with both actually.

dawidwys · 2019-03-05T15:11:51Z

flink-core/src/main/java/org/apache/flink/configuration/Configuration.java

+		} catch (final IllegalArgumentException | NullPointerException e) {
+			final String errorMessage = String.format("Value for config option %s must be one of %s (was %s)",
+				configOption.key(),
+				Arrays.asList(enumClass.getEnumConstants()),


nit:

Suggested change

Arrays.asList(enumClass.getEnumConstants()),

Arrays.toString(enumClass.getEnumConstants()),

That's better. Fixed it.

dawidwys · 2019-03-05T15:17:31Z

flink-core/src/test/java/org/apache/flink/configuration/ConfigurationTest.java

+			configuration.getAsEnum(TestEnum.class, validOption);
+			fail("Expected exception not thrown");
+		} catch (IllegalArgumentException e) {
+			final String expectedMessage = "Value for config option " +


nit: Personally prefer the approach with @Rule

@Rule public ExpectedException thrown = ExpectedException.none(); @Test public testMethod() { ... thrown.expect(IllegalArgumentException.class); thrown.message(....); }

Current solution is legible, and frequently used in Flink and elsewhere. I'll leave it as is.

dawidwys · 2019-03-05T15:25:19Z

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

+				true);
+			fail("Expected exception not thrown");
+		} catch (final IllegalArgumentException e) {
+			assertThat(e.getMessage(), containsString("cannot be set to DISABLED anymore"));


nit: As before -> I prefer the @Rule approach.

dawidwys · 2019-03-05T15:32:30Z

flink-core/src/main/java/org/apache/flink/configuration/Configuration.java

+
+		final String configValue = getString(configOption);
+		try {
+			return T.valueOf(enumClass, configValue);


Shouldn't we call toUpperCase on configValue? At least for the scope of yarn.per-job-cluster.include-user-jar we tried to be case insensitive so far, which seems fair.

You are right. I added a test case for this.

dawidwys · 2019-03-05T15:35:48Z

@flinkbot approve-until architecture

dawidwys · 2019-03-05T16:20:45Z

@flinkbot approve all

+1 LGTM

…job-cluster.include-user-jar Remove this feature because it is broken since Flink 1.5 This closes apache#7883.

GJL · 2019-03-05T17:17:19Z

Thanks for the review @dawidwys. Merging as soon as build is green.

…job-cluster.include-user-jar Remove this feature because it is broken since Flink 1.5 This closes apache#7883.

rmetzger added the review=description? label Mar 3, 2019

zentol reviewed Mar 3, 2019

View reviewed changes

GJL force-pushed the FLINK-11781 branch 2 times, most recently from d157650 to af3e742 Compare March 5, 2019 14:58

dawidwys requested changes Mar 5, 2019

View reviewed changes

rmetzger added review=quality? and removed review=description? labels Mar 5, 2019

dawidwys approved these changes Mar 5, 2019

View reviewed changes

rmetzger added review=approved ✅ and removed review=quality? labels Mar 5, 2019

GJL added a commit to GJL/flink that referenced this pull request Mar 5, 2019

[FLINK-11781][yarn] Remove "DISABLED" as possible value for yarn.per-…

2607039

…job-cluster.include-user-jar Remove this feature because it is broken since Flink 1.5 This closes apache#7883.

GJL added a commit to GJL/flink that referenced this pull request Mar 5, 2019

[FLINK-11781][yarn] Remove "DISABLED" as possible value for yarn.per-…

0ff6b7a

…job-cluster.include-user-jar Remove this feature because it is broken since Flink 1.5 This closes apache#7883.

GJL added a commit to GJL/flink that referenced this pull request Mar 5, 2019

[FLINK-11781][yarn] Remove "DISABLED" as possible value for yarn.per-…

ea64180

…job-cluster.include-user-jar Remove this feature because it is broken since Flink 1.5 This closes apache#7883.

GJL added 2 commits March 6, 2019 08:45

[hotfix][runtime] Remove unused local variable in ExecutionGraph

9ac14a6

[FLINK-11781][yarn] Remove "DISABLED" as possible value for yarn.per-…

6f84008

…job-cluster.include-user-jar Remove this feature because it is broken since Flink 1.5 This closes apache#7883.

GJL force-pushed the FLINK-11781 branch from f6aafc1 to 6f84008 Compare March 6, 2019 08:32

asfgit merged commit 6f84008 into apache:master Mar 6, 2019

rmetzger added the component=Deployment/YARN label Mar 18, 2019

HuangZhenQiu pushed a commit to HuangZhenQiu/flink that referenced this pull request Apr 22, 2019

[FLINK-11781][yarn] Remove "DISABLED" as possible value for yarn.per-…

d8b505b

…job-cluster.include-user-jar Remove this feature because it is broken since Flink 1.5 This closes apache#7883.

GJL deleted the FLINK-11781 branch September 3, 2019 12:06

	return T.valueOf(enumClass, configValue);
	return Enum.valueOf(enumClass, configValue);

	public <T extends Enum<T>> T getAsEnum(
	public <T extends Enum<T>> T getEnum(

	Arrays.asList(enumClass.getEnumConstants()),
	Arrays.toString(enumClass.getEnumConstants()),

Conversation

GJL commented Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

flinkbot commented Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Progress

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GJL Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawidwys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawidwys commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dawidwys commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GJL commented Mar 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

GJL commented Mar 3, 2019 •

edited

Loading

flinkbot commented Mar 3, 2019 •

edited

Loading

GJL Mar 4, 2019 •

edited

Loading

dawidwys commented Mar 5, 2019 •

edited

Loading

dawidwys commented Mar 5, 2019 •

edited

Loading