[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

stupid autospacing

mynameborat

I think overall the PR looks fine. General comments

Please remove any TODOs if you are not planning to address them in the PR or short-term
Mostly minor comments on code style.
Update the status of the PR from draft.

internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java

mynameborat · 2025-02-03T13:29:16Z

internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java

+  /**
+   * Number of threads to use for scheduled log compaction
+   */
+  public static final String SCHEDULED_LOG_COMPACTION_THREAD_COUNT = "scheduled.log.compaction.thread.count";
+
+  /**
+   * Time between each scheduled log compaction
+   */
+  public static final String SCHEDULED_LOG_COMPACTION_INTERVAL_MS = "scheduled.log.compaction.interval.ms";


Ping on this comment.

...nal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestHybrid.java

...egrationTest/java/com/linkedin/venice/endToEnd/logcompaction/TestRepushOrchestratorImpl.java

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

mynameborat · 2025-02-03T13:45:56Z

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

+    try {
+      if (!executor.awaitTermination(SCHEDULED_EXECUTOR_TIMEOUT_S, TimeUnit.SECONDS)) {
+        executor.shutdownNow();
+        LOGGER.info("log compaction service shut down gracefully");


Update log statement to describe timeout to make it explicit.

Do you mean describe how long shutdown took?

it says shut down gracefully but in reality it is attempting to shut down forcefully because waiting for normal shutdown failed due to timeout.

Something along the lines of Timed out waiting for executor to shutdown. Attempting to forcefully close or shut it down

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

...venice-controller/src/main/java/com/linkedin/venice/controller/repush/RepushJobResponse.java

mynameborat · 2025-02-03T13:56:04Z

...venice-controller/src/main/java/com/linkedin/venice/controller/repush/RepushJobResponse.java

+public class RepushJobResponse {
+  private final String storeName;
+  private final String execId;
+  private final String execUrl;


s/execId/executionId and same for the rest.

Also, how is execUrl used? Consider adding another API in RepushOrchestrator that provides status for a given executionId. The status API can provide additional information like,

when it was launched

what store it is for

which concrete orchestrator platform it uses

weblink
etc

execUrl is the link to the AirflowUI, referring to its usage in SmartAssistantHandler

// construct the URL to the Airflow UI return String.format("%s/admin/airflow/graph?dag_id=%s&execution_date=%s&run_id=%s", airflowClient.getBasePath(), SMART_ASSISTANT_DAG_ID, result.getLogicalDate(), result.getDagRunId());

https://github.com/linkedin-multiproduct/nuage-venice/pull/696/files#diff-0dd3d578a191ea41c4cd025ddbe113157a3785de4648368e87e139a1047ac997

re status API: do you mean a codepath getRepushStatus() separate from the existing compactStore() endpoint in StoresRoute?

mynameborat · 2025-02-03T13:58:27Z

...ler/src/test/java/com/linkedin/venice/controller/logcompaction/TestLogCompactionService.java

+    verify(admin, times(expectedCompactStoreInvocationCount)).compactStore(storeForCompaction);
+  }
+
+  // TODO: test LogCompactionTask::run() focus on edge cases


Are you adding one?

If any edge cases comes to mind, I welcome your input.

- encapsulate repush configs & dependencies (e.g. Client)

lusong64 · 2025-02-03T23:55:01Z

...nal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestHybrid.java

@@ -136,6 +144,12 @@ public class TestHybrid {
  private static final Logger LOGGER = LogManager.getLogger(TestHybrid.class);
  public static final int STREAMING_RECORD_SIZE = 1024;

+  // Log compaction test constants
+  private static final int TEST_TIMEOUT = 999999; // ms


Using Integer.MAX_VALUE ?

...nal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestHybrid.java

...ntegrationTest/java/com/linkedin/venice/endToEnd/logcompaction/TestLogCompactionService.java

…g/namespace

- already neasted inside TestHybrid

internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java

...ntegrationTest/java/com/linkedin/venice/endToEnd/logcompaction/TestLogCompactionService.java

- repushOrchestrator creation failure exception - compactStore() compaction failure exception

...nal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestHybrid.java

- dummy config values for new repush configs - decrease latch countdown wait time

- change RepushOrchestrator initialisation in VeniceHelixAdmin & TestHybrid

- RepushJobResponse::storeName -> ControllerResponse::name - set response error

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

[format] comment spacing

1ddc155

stupid autospacing

mynameborat reviewed Feb 3, 2025

View reviewed changes

[controller] add RepushOrchestratorConfig

b406a60

- encapsulate repush configs & dependencies (e.g. Client)

lusong64 reviewed Feb 3, 2025

View reviewed changes

...nal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestHybrid.java Outdated Show resolved Hide resolved

lusong64 reviewed Feb 3, 2025

View reviewed changes

...ntegrationTest/java/com/linkedin/venice/endToEnd/logcompaction/TestLogCompactionService.java Show resolved Hide resolved

Whitney Deng added 2 commits February 3, 2025 16:00

[controller] prepend "controller.repush." to config for prefix scopin…

9150a95

…g/namespace

[controller] remove TestRepushOrchestratorImpl

2b4d2b2

- already neasted inside TestHybrid

arjun4084346 reviewed Feb 4, 2025

View reviewed changes

internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java Show resolved Hide resolved

arjun4084346 reviewed Feb 4, 2025

View reviewed changes

...ntegrationTest/java/com/linkedin/venice/endToEnd/logcompaction/TestLogCompactionService.java Outdated Show resolved Hide resolved

Whitney Deng added 9 commits February 4, 2025 10:39

[controller] embed LogCompactionService in log message

88c9e95

[controller] camelcase fix

5b9be5a

[style] save config key strings as configs

f167714

[controller] upstream exception handling for compactStore()

2df5baf

[style] errorHandlingWrapper() folded into run()

65f121c

[style] match config variable and string name

1a18f0e

[style] append L to long

654444a

[controller][config] remove Client in RepushOrchestratorConfig

8bac2f4

[controller] exception handling

7c7ff8b

- repushOrchestrator creation failure exception - compactStore() compaction failure exception

lusong64 requested changes Feb 6, 2025

View reviewed changes

...nal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestHybrid.java Outdated Show resolved Hide resolved

Whitney Deng added 6 commits February 6, 2025 15:58

[test] update test for new configs & PR comments

c44671f

- dummy config values for new repush configs - decrease latch countdown wait time

[test] [style] decrease scope of CountDownLatch for encapsulation

f577cc7

[controller] add init(config) to RepushOrchestrator

d511e2f

- change RepushOrchestrator initialisation in VeniceHelixAdmin & TestHybrid

cleanup RepushJobResponse fields

589ba1b

[style] delete comment

f474d4f

[controller] RepushJobResponse extends ControllerResponse

0a618c3

- RepushJobResponse::storeName -> ControllerResponse::name - set response error

WhitneyDeng marked this pull request as ready for review February 11, 2025 18:02

WhitneyDeng and others added 3 commits February 11, 2025 10:37

Merge branch 'main' into log-compaction

35ef6a8

[pr] [style] readable names

55d765c

[pr] [style] readable names

0591b60

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 Feb 3, 2025

mynameborat Feb 3, 2025

WhitneyDeng Feb 4, 2025

mynameborat Feb 11, 2025

mynameborat Feb 3, 2025

WhitneyDeng Feb 5, 2025 •

edited

Loading

WhitneyDeng Feb 5, 2025 •

edited

Loading

mynameborat Feb 3, 2025

WhitneyDeng Feb 5, 2025

lusong64 Feb 3, 2025

[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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhitneyDeng Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

WhitneyDeng Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhitneyDeng Feb 5, 2025 •

edited

Loading

WhitneyDeng Feb 5, 2025 •

edited

Loading