Skip to content

[controller][test] Remove legacy log.compaction.threshold.ms config - #2888

Closed
pthirun wants to merge 1 commit into
linkedin:mainfrom
pthirun:pthirun/remove-legacy-log-compaction-threshold-config
Closed

[controller][test] Remove legacy log.compaction.threshold.ms config#2888
pthirun wants to merge 1 commit into
linkedin:mainfrom
pthirun:pthirun/remove-legacy-log-compaction-threshold-config

Conversation

@pthirun

@pthirun pthirun commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Problem Statement

The log-compaction version-staleness threshold is configured by two keys: the current log.compaction.version.staleness.threshold.ms and a legacy log.compaction.threshold.ms. VeniceControllerClusterConfig reads the current key first and only falls back to the legacy one:

this.logCompactionVersionStalenessThresholdMS = props.getLong(
    LOG_COMPACTION_VERSION_STALENESS_THRESHOLD_MS,
    props.getLong(LOG_COMPACTION_THRESHOLD_MS, TimeUnit.HOURS.toMillis(24)));

For any deployment that sets the current key, the legacy key is dead. Keeping two keys for the same value is a maintenance hazard: an operator can set the legacy key and have it silently shadowed by the current one (or by the default).

Solution

  • Remove LOG_COMPACTION_THRESHOLD_MS from ConfigKeys and drop the fallback read in VeniceControllerClusterConfig. The threshold now reads only log.compaction.version.staleness.threshold.ms, defaulting to 24h.
  • Migrate the TestHybrid integration test off the legacy key onto LOG_COMPACTION_VERSION_STALENESS_THRESHOLD_MS, and rename its local constant to match.

No behavior change for deployments already on the current key; the default stays 24h.

Code changes

  • Added new code behind a config. If so list the config names and their default values in the PR description.
  • Introduced new log lines.
    • Confirmed if logs need to be rate limited to avoid excessive logging.

Concurrency-Specific Checks

Not applicable. Config-key removal only.

How was this PR tested?

  • Modified or extended existing tests.

TestHybrid migrated to the current key. Built venice-common, venice-controller, and the venice-test-common integration-test source set; TestVeniceControllerClusterConfig passes; spotlessCheck passes.

Does this PR introduce any user-facing or breaking changes?

  • Yes. Deployments still setting log.compaction.threshold.ms must switch to log.compaction.version.staleness.threshold.ms. After this change the legacy key is ignored, so such a deployment falls back to the 24h default instead of its configured value.

The version-staleness threshold has two config keys: the current
log.compaction.version.staleness.threshold.ms and a legacy
log.compaction.threshold.ms that VeniceControllerClusterConfig only consulted
as a fallback (getLong(VERSION_STALENESS, getLong(THRESHOLD_MS, 24h))). The
version-staleness key is read first, so the legacy key is dead for any
deployment that sets the new one.

Remove LOG_COMPACTION_THRESHOLD_MS and its fallback read, and migrate the
TestHybrid integration test onto
LOG_COMPACTION_VERSION_STALENESS_THRESHOLD_MS.

Breaking: deployments still setting log.compaction.threshold.ms must switch to
log.compaction.version.staleness.threshold.ms, otherwise the threshold falls
back to the 24h default.
@github-actions

Copy link
Copy Markdown

Hi there. This pull request has been inactive for 30 days. To keep our review queue healthy, we plan to close it in 7 days unless there is new activity. If you are still working on this, please push a commit, leave a comment, or convert it to draft to signal intent. Thank you for your time and contributions.

@github-actions github-actions Bot added the stale label Jul 25, 2026
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Closing this pull request due to 37 days of inactivity. This is not a judgment on the value of the work. If you would like to continue, please reopen or open a new PR and we will be happy to take another look. Thank you again for contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant