HuangXingBo commented on code in PR #21110:
URL: https://github.com/apache/flink/pull/21110#discussion_r1001310191
##########
docs/layouts/shortcodes/generated/python_configuration.html:
##########
@@ -110,5 +110,11 @@
<td>Integer</td>
<td>The maximum number of states cached in a Python UDF worker.
Note that this is an experimental flag and might not be available in future
releases.</td>
</tr>
+ <tr>
+ <td><h5>python.systemenv.enabled</h5></td>
+ <td style="word-wrap: break-word;">true</td>
+ <td>Boolean</td>
+ <td>When it is false, system env for Python will be disabled.</td>
Review Comment:
```suggestion
<td>Specify whether to load System Environment when starting
Python worker.</td>
```
##########
flink-python/src/main/java/org/apache/flink/python/env/AbstractPythonEnvironmentManager.java:
##########
@@ -101,9 +104,11 @@ public AbstractPythonEnvironmentManager(
PythonDependencyInfo dependencyInfo,
String[] tmpDirectories,
Map<String, String> systemEnv,
- JobID jobID) {
+ JobID jobID,
+ Configuration config) {
this.dependencyInfo = Objects.requireNonNull(dependencyInfo);
this.tmpDirectories = Objects.requireNonNull(tmpDirectories);
+ this.systemEnvEnabled =
config.get(PythonOptions.PYTHON_SYSTEMENV_ENABLED);
Review Comment:
We can move this to the `createPythonEnvironmentManager` so that we can
decide whether systemEnv is a empty Map
##########
flink-python/src/main/java/org/apache/flink/python/env/AbstractPythonEnvironmentManager.java:
##########
@@ -101,9 +104,11 @@ public AbstractPythonEnvironmentManager(
PythonDependencyInfo dependencyInfo,
String[] tmpDirectories,
Map<String, String> systemEnv,
- JobID jobID) {
+ JobID jobID,
+ Configuration config) {
Review Comment:
We don't need to pass in the config parameter
##########
flink-python/src/main/java/org/apache/flink/python/env/embedded/EmbeddedPythonEnvironmentManager.java:
##########
@@ -42,8 +43,9 @@ public EmbeddedPythonEnvironmentManager(
PythonDependencyInfo dependencyInfo,
String[] tmpDirectories,
Map<String, String> systemEnv,
- JobID jobID) {
- super(dependencyInfo, tmpDirectories, systemEnv, jobID);
+ JobID jobID,
+ Configuration config) {
Review Comment:
ditto
##########
flink-python/src/test/java/org/apache/flink/python/PythonOptionsTest.java:
##########
@@ -172,4 +172,20 @@ void testPythonClientExecutable() {
configuration.get(PythonOptions.PYTHON_CLIENT_EXECUTABLE);
assertThat(actualPythonClientExecutable).isEqualTo(expectedPythonClientExecutable);
}
+
+ @Test
+ void testPythonSystemEnvEnabled() {
Review Comment:
I don't think this test has any value. If we want to test, we should test
whether the `systemEnv` obtained by the operator is different when the config
is in effect.
##########
flink-python/src/main/java/org/apache/flink/python/PythonOptions.java:
##########
@@ -83,6 +83,13 @@ public class PythonOptions {
+ "The interval between each profiling is
determined by the config options "
+ "python.fn-execution.bundle.size and
python.fn-execution.bundle.time.");
+ /** The configuration to enable or disable system env for Python
execution. */
+ public static final ConfigOption<Boolean> PYTHON_SYSTEMENV_ENABLED =
+ ConfigOptions.key("python.systemenv.enabled")
+ .booleanType()
+ .defaultValue(true)
+ .withDescription("When it is false, system env for Python
will be disabled.");
Review Comment:
ditto
##########
flink-python/src/main/java/org/apache/flink/python/env/AbstractPythonEnvironmentManager.java:
##########
@@ -73,6 +75,7 @@ public abstract class AbstractPythonEnvironmentManager
implements PythonEnvironm
protected final PythonDependencyInfo dependencyInfo;
private final Map<String, String> systemEnv;
+ private final boolean systemEnvEnabled;
Review Comment:
we don't need this
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
