[hotfix][clients] Refactor PackagedProgram#getJobJarAndDependencies to reduce some redundant lines.

[RocMarshal]

What is the purpose of the change

[hotfix][clients] Refactor PackagedProgram#getJobJarAndDependencies to reduce some redundant lines.

Brief change log

[hotfix][clients] Refactor PackagedProgram#getJobJarAndDependencies to reduce some redundant lines.

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, Kubernetes/Yarn, 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]

CI report:

• f83e4f4 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[davidradl]

nit: I am curious why we initialize the list with a + 1. Is this in case we need to add the Python lib?
If so, I wonder if we should use isPython to allocate the correct size - ie. without Python we would not have the + 1.

[RocMarshal]

Thanks @davidradl for the comments, That sounds good to me.
I updated it. PTAL :)

[1996fanrui]

I prefer to hardcode it to extractedTempLibraries.size() + 2 instead of checking isPython or not for 2 reasons:

1. The expected list size still needs to +1 if (jarFile != null), so I don't think check isPython is enough if we'd like to initialize a correct size.
2. If we'd like to initialize a proper list size, we need to consider both jarFile and isPython.
   • However, I think it's pretty complicated if checking both.
   • The code base is client related code base instead of data processing, so it's not performance sensitive code.

As performance-awareness part[1] mentioned in Flink code style document:

We can conceptually distinguish between code that “coordinates” and code that “processes data”. 
Code that coordinates should always favor simplicity and cleanness. 
Data processing code is highly performance critical and should optimize for performance.

I agree it, the simplicity and cleanness are more important than performance for coordinates related code. So a simple implementation is + 2 directly. It only requires a little little memory, and it's compatible with both jarFile and isPython.

[1] https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#performance-awareness

[RocMarshal]

Hi, @1996fanrui Could you help take a look at the refactor?
Many thx! :)

[1996fanrui]

Overall LGTM, and left a minor comment~

[1996fanrui]

I prefer to hardcode it to extractedTempLibraries.size() + 2 instead of checking isPython or not for 2 reasons:

1. The expected list size still needs to +1 if (jarFile != null), so I don't think check isPython is enough if we'd like to initialize a correct size.
2. If we'd like to initialize a proper list size, we need to consider both jarFile and isPython.
   • However, I think it's pretty complicated if checking both.
   • The code base is client related code base instead of data processing, so it's not performance sensitive code.

As performance-awareness part[1] mentioned in Flink code style document:

We can conceptually distinguish between code that “coordinates” and code that “processes data”. 
Code that coordinates should always favor simplicity and cleanness. 
Data processing code is highly performance critical and should optimize for performance.

I agree it, the simplicity and cleanness are more important than performance for coordinates related code. So a simple implementation is + 2 directly. It only requires a little little memory, and it's compatible with both jarFile and isPython.

[1] https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#performance-awareness

[RocMarshal]

Thank you @1996fanrui for your comments and explanations.
Nice idea! 👍
This change makes sense to me.
Updated.

[RocMarshal]

@flinkbot run azure

[RocMarshal]

Hi， @davidradl could you help have a double-check ? Because your suggestions are important & indispensable to the pr. 👍

Any input is appreciated!

[1996fanrui]

Let me merge it now as no more comments for over a week~

[RocMarshal]

Thank you for your attention and review. @1996fanrui @davidradl