1996fanrui commented on code in PR #26238:
URL: https://github.com/apache/flink/pull/26238#discussion_r1998041542
##########
flink-clients/src/main/java/org/apache/flink/client/program/PackagedProgram.java:
##########
@@ -246,7 +246,12 @@ public ClassLoader getUserCodeClassLoader() {
/** Returns all provided libraries needed to run the program. */
public List<URL> getJobJarAndDependencies() {
- List<URL> libs = new ArrayList<URL>(extractedTempLibraries.size() + 1);
+ return getJobJarAndDependencies(jarFile, extractedTempLibraries,
isPython);
+ }
+
+ private static List<URL> getJobJarAndDependencies(
+ URL jarFile, List<File> extractedTempLibraries, boolean isPython) {
+ List<URL> libs = new ArrayList<>(extractedTempLibraries.size() +
(isPython ? 1 : 0));
Review Comment:
```suggestion
private static List<URL> getJobJarAndDependencies(
URL jarFile, List<File> extractedTempLibraries, boolean
isPython) {
List<URL> libs = new ArrayList<>(extractedTempLibraries.size() + 2);
```
##########
flink-clients/src/main/java/org/apache/flink/client/program/PackagedProgram.java:
##########
@@ -246,7 +246,18 @@ public ClassLoader getUserCodeClassLoader() {
/** Returns all provided libraries needed to run the program. */
public List<URL> getJobJarAndDependencies() {
- List<URL> libs = new ArrayList<URL>(extractedTempLibraries.size() + 1);
+ List<URL> libs = getJobJarAndDependencies(jarFile,
extractedTempLibraries);
+
+ if (isPython) {
+ libs.add(PackagedProgramUtils.getPythonJar());
+ }
+
+ return libs;
+ }
+
+ private static List<URL> getJobJarAndDependencies(
+ URL jarFile, List<File> extractedTempLibraries) {
+ List<URL> libs = new ArrayList<>(extractedTempLibraries.size() + 1);
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
