[controller] log compaction #1282

WhitneyDeng · 2024-11-05T00:00:12Z

Summary

purpose: to get stores ready for compaction based on programmatically defined criteria
added API endpoints to get compaction-ready stores

How was this PR tested?

unit test: filter all stores on a cluster for compaction-ready stores

Does this PR introduce any user-facing changes?

No. You can skip the rest of this section.
Yes. Make sure to explain your proposed changes and call out the behavior change.

…er methods VeniceParentHelixAdmin::isCompactionReady VeniceParentHelixAdmin::isLastCompactionTimeOlderThanThresholdHours

…or readability & maintainability

…ForCompaction & VeniceParentHelixAdmin::isLastCompactionTimeOlderThanThresholdHours JavaDocs description - complete description of VeniceParentHelixAdmin::getStoresForCompaction - future tense to present tense for VeniceParentHelixAdmin::isLastCompactionTimeOlderThanThresholdHours

…ionReady & helper methods

…ersion is not found

…:filterStoresForCompaction - extract filterStoresForCompaction() logic for testing - write VeniceParentHelixAdminTest::testFilterStoresForCompaction

… TestVeniceParentHelixAdmin - migrate from integration test files to unit test files

… argument order

mynameborat

Good job on putting up the first PR quickly. Looks very good. I have put in some comments.

Let me know if you have questions.

internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerClient.java

...mmon/src/integrationTest/java/com/linkedin/venice/controller/VeniceParentHelixAdminTest.java

services/venice-controller/src/main/java/com/linkedin/venice/controller/Admin.java

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

.../venice-controller/src/main/java/com/linkedin/venice/controller/server/AdminSparkServer.java

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

...nice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceParentHelixAdmin.java

…tionReady package private

…data type ArrayList -> List

- remodularise VeniceParentHelixAdmin::isHybridStore to function var in VeniceParentHelixAdmin::isCompactionReady - make VeniceParentHelixAdmin::isLastCompactionTimeOlderThanThresholdHours hardcoded int to class constant

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

- change exception message in child controller

interface for repush: Azkaban/Airflow implementation for Linkedin internal. (potentially) Spark implementation for OSS.

…iceController

…logic to VeniceHelixAdmin

- empty implementation for now

reason: since repush will be used for diff use cases (other than compaction), the term repush should be reserved for the abstracted repush action.

- instantiate in VeniceController

- this config determines the number of threads for LogCompactionService::executor

…UNT annotations & comments

…into CompactionManager - create CompactionManager & TestCompactionManager - move TestCompactionManager::testFilterStoresForCompaction from TestVeniceParentHelixAdmin

…utor setup in LogCompactionService::startInner

…g time unit from hour to millisecond

- add RepushOrchestratorClassName config (to configure RepushOrchestrator implementation used)

- CompactionManager::filterStoresForCompaction return ArrayList of stores - update TestCompactionManager::testFilterStoresForCompaction - create & organise logcompaction package - update import statements for logcompaction package classes - create RepushJobResponse - RepushOrchestrator::repush returns RepushJobResponse - CompactionManager::compactStore receives RepushResponse -> logs success OR error

mynameborat

I took another pass and put in comments. Looks like some parts are still work in progress. Can you ping me when you have them ready for review?

internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerClient.java

services/venice-controller/src/main/java/com/linkedin/venice/controller/Admin.java

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java

...e-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerClusterConfig.java

...troller/src/main/java/com/linkedin/venice/controller/logcompaction/LogCompactionService.java

mynameborat · 2024-12-05T15:16:28Z

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

+
+  @Override
+  public void compactStore(String storeName) {
+    // TODO


If we are still working on the compaction manager and this method? Can you let me know when this part is ready for review?

Can I confirm that VPHA::compactStore() does nothing since the compaction is done by child controllers?

This is the plan VHA::compactStore() only calls CompactionManager::compactStore() and let CompactionManager handle the trigger response & logging. Any comments/thoughts?

Depends on what sort of thing happens within CompactionManager::compactStore. E.g., if you are invoking other APIs to get some other store metadata. My guess is it should happen on the child controller and also the leader child controller (potentially) to make sure it sees the most up-to-date store metadata e.g., version information (if at all it needs to invalidate some of the request parameters).

Let's have CompactionManager::compactStore log the response status but have VHA::compactStore receive and handle errors.

We can think of CompactionManager as an "irresponsible" class with low visibility, and all it does is execute and hands off any results back upstream.

mynameborat · 2024-12-05T15:19:04Z

...ices/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java

+   * @see Admin#compactStore(String)
+   */
+  public Route compactStore(Admin admin) {
+    return new VeniceRouteHandler<ControllerResponse>(ControllerResponse.class) {


What is the concrete response for the compactStore? Also, this might be not necessary for the log compaction itself.

You can start w/

Implementing CompactionManager logic

Followed by injecting CompactionManager in VeniceHelixAdmin

Tackle the API (controller) last as it is only needed for adhoc triggers and not for LogCompactionService

Putting up the priority above so that you can get some part of the PR ready for review

...venice-controller/src/test/java/com/linkedin/venice/controller/TestLogCompactionService.java

…actionManager

- create com.linkedin.venice.controller.logcompaction in tests - change CompactionManager::filterStoresForCompaction from public to package private

…g-compaction-repush`

WhitneyDeng · 2024-12-06T22:18:32Z

closed because branch renamed to whdeng/log-compaction-repush to follow branch naming convention

WhitneyDeng · 2024-12-06T23:07:13Z

reverted branch name to log-compaction for PR continuity.

…epush` to `log-compaction`

…abled feature flat purpose: control rollout of log compaction service

…ionEnabled

- log trigger source (for now, can expand later)

…QueryParams initialisation to take explicit clusterName directly

- since triggerRepush() is beyond the original scope of log compaction, this method is parked for later addition. adding TODO to remember thought process & design decision to use byte[] + data model class rather than QueryParams for repush job details

- CompactionManager::compactStore & VeniceHelixAdmin::compactStore logs & rethrows exception from repush - LogCompactionService logs exception & can be extended to perform other actions to handle exception - RepushJobResponse will be passe from RepushOrchestrator::repush through CompactionManager::compactStore & VeniceHelixAdmin::compactStore to LogCompactionService - added execution URL to RepushJobResponse, to follow repush.py's pattern

Whitney Deng added 10 commits October 29, 2024 11:52

[controller] log compaction

ff3c1ae

[controller][server] complete log compaction candidate selection help…

6a7903d

…er methods VeniceParentHelixAdmin::isCompactionReady VeniceParentHelixAdmin::isLastCompactionTimeOlderThanThresholdHours

[controller][server][style] put log compaction criterion on newline f…

62fcf64

…or readability & maintainability

[controller][server][documentation] VeniceParentHelixAdmin::isCompact…

8101408

…ionReady & helper methods

[controller][server][documentation] filter out stores where current v…

8160e5b

…ersion is not found

[controller][server][unit test] Unit test for VeniceParentHelixAdmin:…

c28728c

…:filterStoresForCompaction - extract filterStoresForCompaction() logic for testing - write VeniceParentHelixAdminTest::testFilterStoresForCompaction

[controller][server][unit test] move testFilterStoresForCompaction to…

5e481cb

… TestVeniceParentHelixAdmin - migrate from integration test files to unit test files

[controller][server][unit test][style] switch actual & expected value…

2a0f3f2

… argument order

[controller][server][unit test][style] switch actual & expected value…

f6ca080

… argument order

mynameborat reviewed Nov 6, 2024

View reviewed changes

Whitney Deng added 5 commits November 6, 2024 11:38

[controller][server][style][PR] make VeniceParentHelixAdmin::isCompac…

bf73b48

…tionReady package private

[controller][server][style][PR] revert wildcard import

77d83c0

[controller][server][style][PR] delete wildcard import

af12106

[controller][server][PR] change Admin::getStoresForCompaction return …

e82e659

…data type ArrayList -> List

[controller][server][PR]

373c208

- remodularise VeniceParentHelixAdmin::isHybridStore to function var in VeniceParentHelixAdmin::isCompactionReady - make VeniceParentHelixAdmin::isLastCompactionTimeOlderThanThresholdHours hardcoded int to class constant

majisourav99 reviewed Nov 7, 2024

View reviewed changes

...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java Outdated Show resolved Hide resolved

Whitney Deng added 13 commits November 21, 2024 09:10

[controller]

2b11f9d

- change exception message in child controller

[controller] create RepushOrchestratorProvider

1a7f015

interface for repush: Azkaban/Airflow implementation for Linkedin internal. (potentially) Spark implementation for OSS.

[controller] create LogCompactionService & initialise instance in Ven…

1df10d9

…iceController

[controller][PR] change ArrayList to List datatype in contracts

4652dc9

[controller][PR] move VeniceParentHelixAdmin::getStoresForCompaction …

da5455c

…logic to VeniceHelixAdmin

[controller] add triggerRepush endpoint

f30f705

- empty implementation for now

[controller] change repush terminology to compactStore

24c97a1

reason: since repush will be used for diff use cases (other than compaction), the term repush should be reserved for the abstracted repush action.

[controller] create LogCompactionService

3991b22

- instantiate in VeniceController

[controller][config] add SCHEDULED_LOG_COMPACTION_THREAD_COUNT config

43f5c40

- this config determines the number of threads for LogCompactionService::executor

[controller][config][style] delete SCHEDULED_LOG_COMPACTION_THREAD_CO…

f638e40

…UNT annotations & comments

[controller] Refactor VeniceHelixAdmin::getStoresForCompaction logic …

2f3966d

…into CompactionManager - create CompactionManager & TestCompactionManager - move TestCompactionManager::testFilterStoresForCompaction from TestVeniceParentHelixAdmin

[controller] add scheduled.log.compaction.interval.HR config for exec…

8655b96

…utor setup in LogCompactionService::startInner

[controller][config] change schedule.log.compaction.interval.ms confi…

1f44e45

…g time unit from hour to millisecond

WhitneyDeng force-pushed the log-compaction branch from 2a84289 to 1f44e45 Compare November 26, 2024 00:38

Whitney Deng added 4 commits November 25, 2024 16:39

[controller][test] TestLogCompactionService::testScheduledExecution

e0bd4c9

[controller][config] change schedule.log.compaction.interval.ms confi…

275b4ec

…g time unit from hour to millisecond

[controller][config] create RepushOrchestrator interface

cf6861b

- add RepushOrchestratorClassName config (to configure RepushOrchestrator implementation used)

mynameborat reviewed Dec 5, 2024

View reviewed changes

[controller] return type ArrayList > List

bcdb3da

WhitneyDeng changed the title ~~[controller] select stores for log compaction~~ [controller] log compaction Dec 6, 2024

Whitney Deng added 3 commits December 5, 2024 16:52

[controller][PR] revert wildcard import

2c65d3c

[controller][PR] prepend DEFAULT to public static final field in Comp…

9a9cc1d

…actionManager

[controller][PR] move logcompaction tests to same package

5457c79

- create com.linkedin.venice.controller.logcompaction in tests - change CompactionManager::filterStoresForCompaction from public to package private

WhitneyDeng closed this Dec 6, 2024

WhitneyDeng deleted the log-compaction branch December 6, 2024 19:25

test commit after renaming branch from log-compaction to `whdeng/lo…

083af27

…g-compaction-repush`

WhitneyDeng restored the log-compaction branch December 6, 2024 22:11

WhitneyDeng reopened this Dec 6, 2024

Whitney Deng added 9 commits December 6, 2024 15:08

test commit after reverting branch name from `whdeng/log-compaction-r…

dc0835e

…epush` to `log-compaction`

[controller][PR] add and update docs and logs

a9a5aeb

[controller][PR] revert wildcard imports

5ac6468

[controller][PR] add VeniceControllerClusterConfig::isLogCompactionEn…

c4dc3de

…abled feature flat purpose: control rollout of log compaction service

[controller][PR] add VeniceControllerMultiClusterConfig::isLogCompact…

7a851d9

…ionEnabled

[controller] add LogCompactionTask::triggerSource

b93c6fc

- log trigger source (for now, can expand later)

[controller][PR] in ControllerClient::getStoresForCompaction, modify …

5a5dddb

…QueryParams initialisation to take explicit clusterName directly

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controller] log compaction #1282

[controller] log compaction #1282

WhitneyDeng commented Nov 5, 2024

mynameborat left a comment

mynameborat left a comment

mynameborat Dec 5, 2024

WhitneyDeng Dec 6, 2024 •

edited

Loading

mynameborat Dec 10, 2024

WhitneyDeng Dec 10, 2024

mynameborat Dec 5, 2024

WhitneyDeng commented Dec 6, 2024

WhitneyDeng commented Dec 6, 2024

[controller] log compaction #1282

Are you sure you want to change the base?

[controller] log compaction #1282

Conversation

WhitneyDeng commented Nov 5, 2024

Summary

How was this PR tested?

Does this PR introduce any user-facing changes?

mynameborat left a comment

Choose a reason for hiding this comment

mynameborat left a comment

Choose a reason for hiding this comment

mynameborat Dec 5, 2024

Choose a reason for hiding this comment

WhitneyDeng Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

mynameborat Dec 10, 2024

Choose a reason for hiding this comment

WhitneyDeng Dec 10, 2024

Choose a reason for hiding this comment

mynameborat Dec 5, 2024

Choose a reason for hiding this comment

WhitneyDeng commented Dec 6, 2024

WhitneyDeng commented Dec 6, 2024

WhitneyDeng Dec 6, 2024 •

edited

Loading