j1wonpark commented on code in PR #4254:
URL: https://github.com/apache/amoro/pull/4254#discussion_r3487056739
##########
amoro-ams/src/main/java/org/apache/amoro/server/DefaultOptimizingService.java:
##########
@@ -181,6 +182,18 @@ private void
loadOptimizingQueues(List<DefaultTableRuntime> tableRuntimeList) {
optimizerGroups.forEach(
group -> {
String groupName = group.getName();
+ // Fail-safe: a persisted group carrying an invalid DRA config (e.g.
manual DB edits)
+ // must not crash AMS. Surface it and fall back to DRA-disabled
behavior.
Review Comment:
The block was meant as a startup fail-safe — the AIP's "never silent"
requirement, so a persisted invalid config would be surfaced rather than
silently ignored.
But as you pointed out, validating inside the queue loader is the wrong
place, and looking closer the block was behaviorally inert: the disable is
already implicit (`isEffectivelyEnabled()` treats an invalid config as disabled
wherever DRA is consumed), and in Phase 1 nothing strictly parses DRA config at
load, so there was no crash to guard against either. Removed it — surfacing
invalid configs as an alertable signal belongs with the metric that lands in
the observability phase.
--
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]
