Copilot commented on code in PR #17243:
URL: https://github.com/apache/pinot/pull/17243#discussion_r2553369017
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java:
##########
@@ -275,15 +279,17 @@ private boolean shouldRunSegmentValidation(Properties
periodicTaskProperties, lo
}
private boolean shouldRepairErrorSegmentsForPartialUpsertOrDedup(Properties
periodicTaskProperties) {
- return
Optional.ofNullable(periodicTaskProperties.getProperty(REPAIR_ERROR_SEGMENTS_FOR_PARTIAL_UPSERT_OR_DEDUP))
- .map(value -> {
- try {
- return Boolean.parseBoolean(value);
- } catch (Exception e) {
- return false;
- }
- })
- .orElse(false);
+ String property =
periodicTaskProperties.getProperty(REPAIR_ERROR_SEGMENTS_FOR_PARTIAL_UPSERT_OR_DEDUP);
+
+ if (property == null) {
+ return
_llcRealtimeSegmentManager.shouldRepairErrorSegmentsForPartialUpsertOrDedup(_disasterRecoveryMode);
+ }
+
+ try {
+ return Boolean.parseBoolean(property);
+ } catch (Exception e) {
Review Comment:
The catch block silently swallows all exceptions and returns `false` without
logging. This makes debugging configuration issues difficult. Consider logging
the exception with appropriate context about the invalid property value before
returning false.
```suggestion
} catch (Exception e) {
LOGGER.warn("Failed to parse property '{}' for '{}'. Returning
false.", property, REPAIR_ERROR_SEGMENTS_FOR_PARTIAL_UPSERT_OR_DEDUP, e);
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -1129,6 +1132,17 @@ public boolean
isAutoResetErrorSegmentsOnValidationEnabled() {
return
getProperty(ControllerPeriodicTasksConf.AUTO_RESET_ERROR_SEGMENTS_VALIDATION,
true);
}
+ public DisasterRecoveryMode getDisasterRecoveryMode() {
+ return
getDisasterRecoveryMode(getProperty(ControllerPeriodicTasksConf.DISASTER_RECOVERY_MODE_CONFIG_KEY));
+ }
+
+ public static DisasterRecoveryMode getDisasterRecoveryMode(@Nullable String
disasterRecoveryModeString) {
+ if (disasterRecoveryModeString == null) {
+ return null;
+ }
+ return DisasterRecoveryMode.valueOf(disasterRecoveryModeString);
Review Comment:
The `valueOf()` call will throw an `IllegalArgumentException` if an invalid
disaster recovery mode string is provided, but the exception message won't
include helpful context about valid values or where the configuration came
from. Consider wrapping this in a try-catch that provides a more descriptive
error message listing valid `DisasterRecoveryMode` values.
```suggestion
try {
return DisasterRecoveryMode.valueOf(disasterRecoveryModeString);
} catch (IllegalArgumentException e) {
String validModes = Arrays.stream(DisasterRecoveryMode.values())
.map(Enum::name)
.collect(Collectors.joining(", "));
throw new IllegalArgumentException(
"Invalid disaster recovery mode: '" + disasterRecoveryModeString
+ "'. Valid values are: [" + validModes + "]."
+ " (from config key: '" +
ControllerPeriodicTasksConf.DISASTER_RECOVERY_MODE_CONFIG_KEY + "')", e);
}
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
