zhoujinsong commented on PR #4112:
URL: https://github.com/apache/amoro/pull/4112#issuecomment-4021776947
Thanks for the contribution! I noticed an inconsistency in the plugin
configuration approach compared to other existing plugins.
**Current approach in this PR:**
The `iceberg-rest-catalog` extension is configured inside `config.yaml`
under the `http-server` section:
```yaml
http-server:
iceberg-rest-catalog:
enabled: true
```
This is achieved by overriding `loadPluginConfigurations()` in
`RestExtensionManager` to read the `enabled` flag directly from the
`Configurations` object, bypassing the standard plugin config mechanism.
**Existing plugin configuration pattern:**
All other plugins (e.g., `metric-reporters`, `event-listeners`,
`table-runtime-factories`) use a dedicated YAML file under
`conf/plugins/<category>.yaml`:
```yaml
# conf/plugins/metric-reporters.yaml
metric-reporters:
- name: prometheus-exporter
enabled: false
properties:
port: 7001
```
This is handled automatically by the default `loadPluginConfigurations()` in
`AbstractPluginManager`, which supports `enabled`, `priority`, and custom
`properties` out of the box.
**Problems with the current approach:**
1. **Inconsistency** — Users familiar with the existing plugin system would
expect to configure REST extensions in `conf/plugins/rest-extension.yaml`, not
buried inside `config.yaml`.
2. **Limited extensibility** — The design only supports an `enabled` toggle.
Third-party `RestExtension` implementations that require additional
configuration (e.g., bind port, auth tokens) have no standard place to declare
their `properties`.
3. **Reinvents the wheel** — `AbstractPluginManager` already provides full
config loading, priority ordering, and lifecycle management. The current
implementation duplicates this logic unnecessarily.
**Suggestion:**
Introduce `conf/plugins/rest-extension.yaml` and rely on the default
`loadPluginConfigurations()` inherited from `AbstractPluginManager`, keeping it
consistent with all other plugin categories. This would also give third-party
extensions a proper way to pass custom properties.
--
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]
