Jackie-Jiang commented on code in PR #15466:
URL: https://github.com/apache/pinot/pull/15466#discussion_r2048000600
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -367,6 +372,8 @@ private static long getRandomInitialDelayInSeconds() {
public static final String EXIT_ON_SCHEMA_CHECK_FAILURE =
"controller.startup.exitOnSchemaCheckFailure";
public static final boolean DEFAULT_EXIT_ON_SCHEMA_CHECK_FAILURE = true;
+ private final Map<String, String> _warnings = new HashMap<>();
Review Comment:
Suggest making it more specific, such as `invalidConfigs`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerHealthCheck.java:
##########
@@ -119,6 +121,20 @@ public long getUptime() {
return uptime.getSeconds();
}
+ @GET
+ @Path("/pinot-controller/invalid/configs")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_HEALTH)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List cluster config warnings")
Review Comment:
It only lists the invalid controller configs
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -87,6 +89,9 @@ public enum ControllerMode {
DUAL, PINOT_ONLY, HELIX_ONLY
}
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ControllerConf.class);
+
+
Review Comment:
Seems not used
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -263,7 +263,6 @@ public void init(PinotConfiguration pinotConfiguration)
"tenant-rebalance-thread-%d");
_tenantRebalancer = new DefaultTenantRebalancer(_helixResourceManager,
_tenantRebalanceExecutorService);
}
-
Review Comment:
(nit) Revert
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -1200,6 +1237,26 @@ private long convertPeriodToSeconds(String period) {
return convertPeriodToUnit(period, TimeUnit.SECONDS);
}
+ private boolean isValidPeriodWithLogging(String propertyKey, String
periodStr) {
+ if (TimeUtils.isPeriodValid(periodStr)) {
+ return true;
+ } else {
+ String message = String.format(
+ "Invalid time spec '%s' for config '%s'. Falling back to default
config.", periodStr,
+ propertyKey);
+ addControllerInvalidConfigs(propertyKey, message);
+ return false;
+ }
+ }
+
+ private void addControllerInvalidConfigs(String propertyKey, String
errorMessage) {
+ _warnings.put(propertyKey, errorMessage);
+ }
+
+ public Map<String, String> getControllerInvalidConfigs() {
Review Comment:
(minor)
```suggestion
public Map<String, String> getInvalidConfigs() {
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -1200,6 +1237,26 @@ private long convertPeriodToSeconds(String period) {
return convertPeriodToUnit(period, TimeUnit.SECONDS);
}
+ private boolean isValidPeriodWithLogging(String propertyKey, String
periodStr) {
+ if (TimeUtils.isPeriodValid(periodStr)) {
+ return true;
+ } else {
+ String message = String.format(
+ "Invalid time spec '%s' for config '%s'. Falling back to default
config.", periodStr,
+ propertyKey);
+ addControllerInvalidConfigs(propertyKey, message);
Review Comment:
(minor) This can be inlined
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerHealthCheck.java:
##########
@@ -119,6 +121,20 @@ public long getUptime() {
return uptime.getSeconds();
}
+ @GET
+ @Path("/pinot-controller/invalid/configs")
Review Comment:
This is an API on controller, so we can simplify it to
```suggestion
@Path("invalidconfigs")
```
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
