apache / apache/cloudstack

remove redundant/diverging hardcoded defaults in parseInt/parseLong wrapping ConfigKey reads

Open
#13,893 3 comments 0 reactions 1 assignee Claimed by @DaanHoogland View on GitHub
type:technical-debt
Dominant language
Java
Stars
3.1k
Forks
1.4k
Avg merge
6d 19h
Merged PRs (30d)
32

Description

Many call sites read a `ConfigKey` via the raw DAO and wrap the result in `NumbersUtil.parseInt(value, someLiteral)` / `NumbersUtil.parseLong(...)` / `Integer.parseInt(...)`, supplying a hardcoded fallback even though the `ConfigKey` already carries its own default via `.defaultValue()` (and `.value()` applies that default automatically when no DB row exists). Most of the time the literal happens to match the `ConfigKey`'s declared default, so it's just redundant. But it doesn't always match — which is a latent bug, not just noise, since the two numbers silently diverge depending on which code path executes.

Found during the `.value()` migration pass (issue #10752):

- `DeploymentPlanningManagerImpl.java` — `HostReservationReleasePeriod`: `ConfigKey` default is `"300000"`, but the field's Java-level initializer is `60L * 60L * 1000L` (3,600,000), and the code only falls back to the `ConfigKey` default when the persisted value is `<= 0` — not when the row is simply missing (null), in which case it silently keeps 3,600,000 instead of 300,000.
- `StorageCacheManagerImpl.java` — `ExpungeWorkers`: `ConfigKey` default is `"1"`, but the read is `NumbersUtil.parseInt(configDao.getValue(...), 10)` — a hardcoded fallback of 10 vs. the registered default of 1.
- `UcsManagerImpl.java` — `UCSSyncBladeInterval`: `ConfigKey` default is `"3600"`, but a leftover `catch (NumberFormatException e) { syncBladeInterval = 600; }` uses 600. Low risk in practice since `.value()` no longer throws on parse, but the mismatched literal is still latent debt.

These three were left un-migrated to `.value()` specifically because of this discrepancy — picking either number without a decision would silently change fresh-install/edge-case behavior. Worth a repo-wide sweep for the same `parseInt`/`parseLong`-with-hardcoded-default pattern, since these three were only found incidentally while migrating unrelated call sites, not through an exhaustive search.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.