[FLINK-14839][config] Let JobGraph#classpaths become non-null #10235

tisonkun · 2019-11-18T10:03:59Z

What is the purpose of the change

JobGraph#userJars & JobGraph#classpaths are non-null, so we can improve our codes a bit.

Brief change log

Let JobGraph#classpaths become non-null. Preciously, we now add classpaths into an ArrayList instead of changing the reference.
Thus, rename setClasspaths to addClasspaths
BlobLibraryCacheManager#registerTask never receives nullable parameter, indeed.

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

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

cc @zentol @GJL

tisonkun · 2019-11-18T10:04:50Z

flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java

-	 *
-	 * @return True, if the JobGraph has user code JAR files attached, false otherwise.
-	 */
-	public boolean hasUsercodeJarFiles() {


flinkbot · 2019-11-18T10:08:38Z

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 71f07dc (Wed Dec 04 15:55: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 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-11-18T10:45:38Z

CI report:

15f1c1a : FAILURE Build
30ef6bc : SUCCESS Build
7c0af1f : FAILURE Build
71f07dc : SUCCESS Build

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build

tisonkun · 2019-11-20T01:55:14Z

@flinkbot run travis

GJL

Looks ok. I have left some remarks.

flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java

GJL · 2019-11-21T16:38:33Z

flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java

@@ -352,8 +353,8 @@ public JobVertex findVertexByID(JobVertexID id) {
 	 *
 	 * @param paths paths of the directories/JAR files required to run the job on a task manager
 	 */
-	public void setClasspaths(List<URL> paths) {
-		classpaths = paths;
+	public void addClasspaths(Collection<URL> paths) {


Are we actually invoking addClasspaths multiple times? It seems that the changeset could be smaller by just adding a null check to the setter.

So for addJars. We don't call addJars multiple times for now. However, it fits the abstraction that we firstly instance an empty collection and then add member per call. If we still using setter and with a null checker the field cannot be final while we don't gain a lot from such mutability.

…ives nullable parameter

tisonkun · 2019-11-26T05:25:21Z

@GJL could you give this patch another pass? I've addressed your comments above.

GJL

I am afraid that we cannot do this change. Let me know if you see a workaround.

GJL · 2019-11-27T10:25:09Z

flink-runtime/src/main/java/org/apache/flink/runtime/jobgraph/JobGraph.java

@@ -100,7 +101,7 @@
 	private final List<PermanentBlobKey> userJarBlobKeys = new ArrayList<>();

 	/** List of classpaths required to run this job. */
-	private List<URL> classpaths = Collections.emptyList();


It's problematic that JobGraph implements Serialized and classpaths is a non-transient field. After a cluster upgrade to 1.10 addClasspaths will throw UnsupportedOperationException if classpath of the serialized JobGraph was previously empty.

OK I can see the problem. Although we can work around by adding another field and add some bridge code I don't it is worth to do so because this is just a nice to have improvement. A reasonable way in my mind is that we take this thing into consideration when we port JobGraph to its protobuf version if there will be one. Anyway, defer the effort until we are in a codebase easy to do it.

tisonkun · 2019-11-27T11:32:27Z

closed as I cannot find a worthy workaround for the problem described in https://github.com/apache/flink/pull/10235/files/71f07dc72c9590bef027993dc258e598137514a7#r351205882

tisonkun commented Nov 18, 2019

View reviewed changes

rmetzger added the review=description? label Nov 18, 2019

rmetzger added the component=Runtime/Configuration label Nov 18, 2019

tisonkun force-pushed the FLINK-14839 branch 2 times, most recently from e1f7257 to 30ef6bc Compare November 20, 2019 02:07

GJL reviewed Nov 21, 2019

View reviewed changes

tisonkun force-pushed the FLINK-14839 branch from 30ef6bc to 7c0af1f Compare November 22, 2019 02:55

tisonkun added 4 commits November 22, 2019 10:55

[FLINK-14839][config] Let JobGraph#classpaths become non-null

95332ad

[FLINK-14839][config] BlobLibraryCacheManager#registerTask never rece…

f50080b

…ives nullable parameter

[hotfix] Return unmodifiableList on JobGraph#getClasspaths

7c0af1f

fixup! [hotfix] Return unmodifiableList on JobGraph#getClasspaths

71f07dc

GJL reviewed Nov 27, 2019

View reviewed changes

tisonkun closed this Nov 27, 2019

tisonkun deleted the FLINK-14839 branch November 27, 2019 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-14839][config] Let JobGraph#classpaths become non-null #10235

[FLINK-14839][config] Let JobGraph#classpaths become non-null #10235

tisonkun commented Nov 18, 2019

tisonkun Nov 18, 2019

flinkbot commented Nov 18, 2019 •

edited

flinkbot commented Nov 18, 2019 •

edited

tisonkun commented Nov 20, 2019

GJL left a comment

GJL Nov 21, 2019

tisonkun Nov 22, 2019

tisonkun commented Nov 26, 2019

GJL left a comment

GJL Nov 27, 2019

tisonkun Nov 27, 2019

tisonkun commented Nov 27, 2019

[FLINK-14839][config] Let JobGraph#classpaths become non-null #10235

[FLINK-14839][config] Let JobGraph#classpaths become non-null #10235

Conversation

tisonkun commented Nov 18, 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

tisonkun Nov 18, 2019

Choose a reason for hiding this comment

flinkbot commented Nov 18, 2019 • edited

Automated Checks

Review Progress

flinkbot commented Nov 18, 2019 • edited

CI report:

tisonkun commented Nov 20, 2019

GJL left a comment

Choose a reason for hiding this comment

GJL Nov 21, 2019

Choose a reason for hiding this comment

tisonkun Nov 22, 2019

Choose a reason for hiding this comment

tisonkun commented Nov 26, 2019

GJL left a comment

Choose a reason for hiding this comment

GJL Nov 27, 2019

Choose a reason for hiding this comment

tisonkun Nov 27, 2019

Choose a reason for hiding this comment

tisonkun commented Nov 27, 2019

flinkbot commented Nov 18, 2019 •

edited

flinkbot commented Nov 18, 2019 •

edited