RexXiong commented on code in PR #2125:
URL:
https://github.com/apache/incubator-celeborn/pull/2125#discussion_r1412764015
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/ConfigService.java:
##########
@@ -17,7 +17,7 @@
package org.apache.celeborn.server.common.service.config;
-public interface ConfigService {
+public interface ConfigService extends AutoCloseable {
Review Comment:
Usually for this type of services have no scenario to use try-with-resource,
I also think this change is unnecessary
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/FsConfigServiceImpl.java:
##########
@@ -88,10 +85,12 @@ private synchronized void refresh() {
}
} catch (Exception e) {
LOG.warn("Refresh dynamic config error: {}", e.getMessage(), e);
+ return;
}
tenantConfigAtomicReference.set(tenantConfs);
- systemConfigAtomicReference.set(systemConfig == null ? new
SystemConfig(celebornConf) : systemConfig);
+ systemConfigAtomicReference.set(
+ systemConfig == null ? new SystemConfig(celebornConf) : systemConfig);
Review Comment:
Actually, the current design treats CelebornConf as the baseConf for
SystemConfig, so that the interfaces of ConfigServer can be unified to
DynamicConf. Of course, it's also possible to have SystemConfig as null, but
that would require ConfigService to implement other interfaces such as
getSystemValue/getTenantValue. However, this may not easy to make a
configuration cache. Therefore, I adopted the first design.
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/FsConfigServiceImpl.java:
##########
@@ -88,10 +85,12 @@ private synchronized void refresh() {
}
} catch (Exception e) {
LOG.warn("Refresh dynamic config error: {}", e.getMessage(), e);
+ return;
Review Comment:
Agree, thanks.
--
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]
