apache / apache/cloudstack

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

Đang mở
#13,898 2 bình luận 0 reaction 1 người được giao Được @DaanHoogland nhận Xem trên GitHub
type:technical-debt
Ngôn ngữ chính
Java
Star
3.1k
Fork
1.4k
Merge trung bình
6 ngày 19 giờ
Pull request đã merge (30 ngày)
32

Mô tả

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.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.