apache / apache/cloudstack

ConfigKeys left unused while their read site still does raw _configDao.getConfiguration() map lookups

Abierto
#13,898 2 comentarios 0 reacciones 1 asignado Reclamado por @DaanHoogland Ver en GitHub
type:technical-debt
Lenguaje dominante
Java
Estrellas
3.1k
Forks
1.4k
Merge medio
6 d 19 h
PR fusionados (30 d)
32

Descripción

Found while writing ConfigKey-wiring unit tests for PR #13884 (issue #10752, phase out enum `Config`).

`ManagementServerImpl.configure()` (server/src/main/java/com/cloud/server/ManagementServerImpl.java:1141-1148) still reads two settings through the raw configuration map instead of their migrated `ConfigKey`s:

```java
_configs = _configDao.getConfiguration();

final String value = _configs.get("event.purge.interval");
final int cleanup = NumbersUtil.parseInt(value, 60 * 60 * 24); // 1 day.

_purgeDelay = NumbersUtil.parseInt(_configs.get("event.purge.delay"), 0);
```

`ManagementServer.EventPurgeInterval` and `ManagementServer.EventPurgeDelay` both already exist on the interface — three lines below, `AlertPurgeInterval`/`AlertPurgeDelay` (the exact same purge-scheduling pattern) were correctly migrated to `.value()` in the same method, so these two are a clear miss rather than a deliberate choice.

This is a different variant from the pattern in #13893 (redundant/diverging hardcoded defaults passed to `parseInt`/`parseLong` around an already-migrated `.value()` call): here the read site was never migrated to `.value()` (or even to `_configDao.getValue(key.key())`) at all — it still goes through a raw `Map _configs = _configDao.getConfiguration()` lookup by literal key string, bypassing the `ConfigKey` entirely. The earlier `.value()`-replacement sweep only searched for the `_configDao.getValue(key.key())` call shape, so it structurally couldn't catch this one.

It also happens to carry the same kind of default-mismatch risk as #13893: `EventPurgeDelay`'s registered default is `"15"`, but the hardcoded fallback here is `0`, and `0` is the sentinel that disables purging (`if (_purgeDelay != 0) { schedule... }`). So migrating this site isn't a pure no-op — it needs the same default-reconciliation judgment call as the #13893 cases before switching it over.

Only these two sites were found in `ManagementServerImpl`; this was spotted incidentally while scoping unit tests for the `.value()` migration, not through an exhaustive sweep — worth a repo-wide grep for other `_configDao.getConfiguration()`-backed raw map reads (`_configs.get("literal.key")`) that have a matching `ConfigKey` sitting unused nearby.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.