apache / apache/cloudstack

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

Ouverte
#13,898 2 commentaires 0 réactions 1 personne assignée Réclamée par @DaanHoogland Voir sur GitHub
type:technical-debt
Langage dominant
Java
Étoiles
3.1k
Forks
1.4k
Merge moyen
6 j 19 h
PR mergées (30 j)
32

Description

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.

Guide de contribution

Ouvrir le guide de contribution

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.