zhang-arvin commented on PR #4347: URL: https://github.com/apache/amoro/pull/4347#issuecomment-6058857658
Thanks for the detailed review @czy006 — you're right, and the blocking point is real. `pluginConfigs.isEmpty()` conflates "config file missing" with "file present but its list is empty", and since the shipped `conf/plugins/metric-reporters.yaml` has its list commented out, the current branch would auto-install the bundled `PrometheusMetricsReporter` with an empty config (`Lack required property: port`) and take AMS down on a fresh install. I'll rework the trigger. Plan, following your direction: - **Narrow the trigger.** Change `loadPluginConfigurations()` to return `Optional<List<PluginConfiguration>>`: file absent -> `Optional.empty()` (auto-install the discovered plugins — the IDE case); file present -> `Optional.of(configs)` even when the list is empty (keep today's "install nothing" semantics). I'll update the `TestPluginManager` override accordingly. - **Don't let one bad plugin abort startup.** In the auto-install path, wrap each `install()` in try/catch, log a WARN and skip the failing plugin, so prometheus-exporter with a missing mandatory property can't break AMS boot. - **Tests** in `TestAbstractPluginManager`: (a) file missing + discovered plugins -> all installed; (b) a discovered plugin whose `open()` throws -> initialize logs a warning and continues. Will push the updated revision shortly. Thanks again for the pointer to `TestAbstractPluginManager`. -- 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]
