Copilot commented on code in PR #5167:
URL: https://github.com/apache/zeppelin/pull/5167#discussion_r2930017638
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java:
##########
@@ -64,10 +70,21 @@ public TimeoutLifecycleManager(ZeppelinConfiguration zConf,
LOGGER.debug("Check idle time of interpreter");
}
}, checkInterval, checkInterval, MILLISECONDS);
- LOGGER.info("TimeoutLifecycleManager is started with checkInterval: {},
timeoutThreshold: ΒΈ{}", checkInterval,
+ LOGGER.info("TimeoutLifecycleManager is started with checkInterval: {},
timeoutThreshold: {}", checkInterval,
timeoutThreshold);
}
+ static long parseTimeValue(String value) {
+ try {
+ return Long.parseLong(value);
+ } catch (NumberFormatException e) {
+ if (value.endsWith("ms")) {
+ return Long.parseLong(value.substring(0, value.length() - 2));
+ }
+ return Duration.parse("PT" + value).toMillis();
+ }
+ }
Review Comment:
`parseTimeValue` will throw for previously-supported values like `2s`, `1m`,
`1h` because `Duration.parse` expects ISO-8601 units in uppercase (e.g.,
`PT2S`, `PT1M`, `PT1H`). This is a behavioral regression vs the old
`ZeppelinConfiguration.timeUnitToMill` tests. Consider implementing explicit
suffix parsing for `ms/s/m/h` (case-insensitive) or normalizing to valid
ISO-8601 before calling `Duration.parse`, and handle `DateTimeParseException`
with a clear fallback/error.
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java:
##########
@@ -41,15 +42,20 @@ public class TimeoutLifecycleManager extends
LifecycleManager {
private static final Logger LOGGER =
LoggerFactory.getLogger(TimeoutLifecycleManager.class);
+ private static final long DEFAULT_CHECK_INTERVAL = 60000L;
+ private static final long DEFAULT_TIMEOUT_THRESHOLD = 3600000L;
+
private long lastBusyTimeInMillis;
- public TimeoutLifecycleManager(ZeppelinConfiguration zConf,
+ public TimeoutLifecycleManager(Properties properties,
RemoteInterpreterServer
remoteInterpreterServer) {
- super(zConf, remoteInterpreterServer);
- long checkInterval = zConf.getTime(ZeppelinConfiguration.ConfVars
- .ZEPPELIN_INTERPRETER_LIFECYCLE_MANAGER_TIMEOUT_CHECK_INTERVAL);
- long timeoutThreshold = zConf.getTime(
-
ZeppelinConfiguration.ConfVars.ZEPPELIN_INTERPRETER_LIFECYCLE_MANAGER_TIMEOUT_THRESHOLD);
+ super(properties, remoteInterpreterServer);
+ long checkInterval = parseTimeValue(properties.getProperty(
+ "zeppelin.interpreter.lifecyclemanager.timeout.checkinterval",
+ String.valueOf(DEFAULT_CHECK_INTERVAL)));
+ long timeoutThreshold = parseTimeValue(properties.getProperty(
+ "zeppelin.interpreter.lifecyclemanager.timeout.threshold",
+ String.valueOf(DEFAULT_TIMEOUT_THRESHOLD)));
Review Comment:
The new time parsing behavior (including unit-suffixed inputs like `10ms`,
`2s`, `1m`, `1h`) is not covered here, and the old unit tests for
`timeUnitToMill` were removed. Add unit tests targeting
`TimeoutLifecycleManager.parseTimeValue` (or a shared replacement utility) to
lock in the accepted formats and prevent regressions.
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java:
##########
@@ -200,10 +199,8 @@ public void run() {
@Override
public void init(Map<String, String> properties) throws
InterpreterRPCException, TException {
- this.zConf = ZeppelinConfiguration.load();
- for (Map.Entry<String, String> entry : properties.entrySet()) {
- this.zConf.setProperty(entry.getKey(), entry.getValue());
- }
+ this.zProperties = new Properties();
+ this.zProperties.putAll(properties);
Review Comment:
`init` no longer seeds configuration from `ZeppelinConfiguration.load()`
(and thus no longer pulls in `zeppelin-site.xml` and any implicit defaults it
provided) before overlaying the passed-in map. Unless all required settings are
now guaranteed to be passed in via `properties`, this can change runtime
behavior (e.g., proxy settings, repo URL, lifecycle settings) in deployed
interpreter processes. A more robust approach is to assemble `zProperties` from
a known baseline (e.g., System properties + expected env vars + explicit
defaults) and then overlay the incoming RPC map.
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/SchedulerFactory.java:
##########
@@ -50,12 +49,23 @@ public static SchedulerFactory singleton() {
}
private SchedulerFactory() {
- int threadPoolSize = ZeppelinConfiguration
-
.getStaticInt(ZeppelinConfiguration.ConfVars.ZEPPELIN_INTERPRETER_SCHEDULER_POOL_SIZE);
+ int threadPoolSize = getSchedulerPoolSize();
LOGGER.info("Scheduler Thread Pool Size: {}", threadPoolSize);
executor =
ExecutorFactory.singleton().createOrGet(SCHEDULER_EXECUTOR_NAME,
threadPoolSize);
}
+ private static int getSchedulerPoolSize() {
+ String envValue =
System.getenv("ZEPPELIN_INTERPRETER_SCHEDULER_POOL_SIZE");
+ if (envValue != null) {
+ return Integer.parseInt(envValue);
+ }
+ String propValue =
System.getProperty("zeppelin.scheduler.threadpool.size");
+ if (propValue != null) {
+ return Integer.parseInt(propValue);
+ }
+ return 100;
Review Comment:
`Integer.parseInt` can throw `NumberFormatException` on misconfiguration and
fail interpreter startup. The previous
`ZeppelinConfiguration.getStaticInt(...)` likely handled parsing/defaulting
more safely. Wrap parsing in a try/catch (and optionally validate bounds > 0)
so invalid values fall back to the default (or log a warning and continue).
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java:
##########
@@ -213,8 +210,8 @@ public void init(Map<String, String> properties) throws
InterpreterRPCException,
}
if (!isTest) {
- int connectionPoolSize =
-
this.zConf.getInt(ZeppelinConfiguration.ConfVars.ZEPPELIN_INTERPRETER_CONNECTION_POOL_SIZE);
+ int connectionPoolSize = Integer.parseInt(
+
zProperties.getProperty("zeppelin.interpreter.connection.poolsize", "100"));
Review Comment:
The same property key + default (`zeppelin.interpreter.connection.poolsize`,
`"100"`) is duplicated in multiple places. Extract this into a single constant
(and ideally a small helper that also handles invalid integer values) to
prevent drift/typos and keep reconnect behavior consistent with init behavior.
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java:
##########
@@ -484,7 +486,7 @@ public void reconnect(String host, int port) throws
InterpreterRPCException, TEx
this.intpEventServerHost = host;
this.intpEventServerPort = port;
intpEventClient = new RemoteInterpreterEventClient(intpEventServerHost,
intpEventServerPort,
-
this.zConf.getInt(ZeppelinConfiguration.ConfVars.ZEPPELIN_INTERPRETER_CONNECTION_POOL_SIZE));
+
Integer.parseInt(zProperties.getProperty("zeppelin.interpreter.connection.poolsize",
"100")));
Review Comment:
The same property key + default (`zeppelin.interpreter.connection.poolsize`,
`"100"`) is duplicated in multiple places. Extract this into a single constant
(and ideally a small helper that also handles invalid integer values) to
prevent drift/typos and keep reconnect behavior consistent with init behavior.
--
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]
