czy006 commented on PR #4347:
URL: https://github.com/apache/amoro/pull/4347#issuecomment-5580599086
Hi @zhang-arvin, thanks a lot for picking this up, and for the solid
investigation in #1523 — your root-cause analysis (plugins are discovered via
ServiceLoader but never installed because the `conf/plugins/` directory is
absent in IDE setups) is spot on. Making the local optimizer work
out-of-the-box from an IDE is definitely worth fixing. 👍
Unfortunately I found one blocking issue with the current trigger condition
that we'll need to address before merging. It's quite contained to fix though,
and I've sketched a direction below — happy to help review the next iteration!
### Blocking: default binary deployments would fail to start
The shipped `conf/plugins/metric-reporters.yaml` on master has its plugin
list fully commented out, so `loadPluginConfigurations()` returns an **empty
list in every default deployment** — not just in IDE runs. And since
`amoro-metrics-prometheus` is bundled in the distribution and registers
`PrometheusMetricsReporter` via SPI, `foundedPlugins` is non-empty, so the new
branch auto-installs it with an empty config.
`PrometheusMetricsReporter.open()` then throws `IllegalArgumentException("Lack
required property: port")`
([PrometheusMetricsReporter.java#L39](https://github.com/apache/amoro/blob/master/amoro-metrics/amoro-metrics-prometheus/src/main/java/org/apache/amoro/metrics/promethues/PrometheusMetricsReporter.java#L39)),
and since nothing on the path catches it (`install()` → `initialize()` →
`MetricManager.getInstance()` → `startRestServices()`), AMS would fail to boot
on a fresh install.
### The root of it: "empty" conflates two different states
`pluginConfigs.isEmpty()` currently means either:
1. the config **file is missing** (the IDE case this PR targets), or
2. the file **exists but its list is empty** — the deliberate way to disable
a whole category, and the shipped default for `metric-reporters`.
For case 2, this change flips the semantics from "install nothing" to
"install everything discovered". For example, an operator who disables the
Iceberg REST catalog by deleting its entry from `rest-extensions.yaml` (rather
than setting `enabled: false`) would get it silently re-enabled with default
properties — that's surprising behavior we should avoid.
### Suggested direction
- Distinguish the two states, e.g. have `loadPluginConfigurations()` return
`Optional<List<PluginConfiguration>>`: file missing → auto-install defaults
(your new branch); file present but list empty → keep today's "install nothing"
behavior. This resolves both points above in one go.
- In the auto-install path, wrap each `install()` in a try/catch that logs a
WARN and skips the failing plugin, so a single plugin that requires mandatory
properties (prometheus-exporter does today) can't abort AMS startup.
- Tests would be very welcome — `TestAbstractPluginManager` already has
everything you need (injectable configs + `TestPluginImplA/B` registered in the
test SPI):
- empty configs + discovered plugins → all installed;
- a discovered plugin whose `open()` throws → initialize logs a warning
and continues.
Thanks again for working on this — the diagnosis was great, and with the
trigger condition narrowed this will be a nice improvement for IDE development!
🙌
--
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]
