8000 refactor: unify `hummock+memory` with `hummock+memory-shared` & fix compactor service in playground by BugenZhao · Pull Request #21847 · risingwavelabs/risingwave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: unify hummock+memory with hummock+memory-shared & fix compactor service in playground #21847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
May 14, 2025
Merged

refactor: unify hummock+memory with hummock+memory-shared & fix compactor service in playground #21847

merged 5 commits into from
May 14, 2025

Conversation

BugenZhao
Copy link
Member
@BugenZhao BugenZhao commented May 13, 2025 •
edited
Loading

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Follow up of #21353.

This PR unifies hummock+memory with hummock+memory-shared: we will always use the singleton in-memory object store in the process. This is because I cannot find an actual use case for differentiating them after removing the distributed in-memory e2e tests.

This PR also fixes a bug that in playground (or single-node with --in-memory) mode, we specify the state store url as non-shared while still starting a compactor, which cannot access the objects as a result.

2025-05-13T17:30:23.558657+08:00 ERROR rw-standalone-compactor risingwave_object_store::object: read failed error=NotFound error: no object at path 'hummock_001/13964.data'
2025-05-13T17:30:23.558679+08:00 ERROR rw-standalone-compactor risingwave_object_store::object: read failed error=NotFound error: no object at path 'hummock_001/13917.data'
2025-05-13T17:30:23.558793+08:00  WARN           rw-compaction risingwave_storage::hummock::compactor::compactor_runner: Compaction task 6264 failed with error error=Foyer error: NotFound error: no object at path 'hummock_001/14027.data'

Given that an embedded compactor will be started by the compute node service, there's no need to start a compactor service separately at all. Previously we will start 2 compactor service due to this reason. This PR fixes this as well.

risingwave/src/compute/src/server.rs

Lines 535 to 541 in 3c87f77

/// Checks whether an embedded compactor starts with a compute node.
fn embedded_compactor_enabled(state_store_url: &str, disable_remote_compactor: bool) -> bool {
// Always start an embedded compactor if the state store is in-memory.
state_store_url.starts_with("hummock+memory")
|| state_store_url.starts_with("hummock+disk")
|| disable_remote_compactor
}

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

@BugenZhao
refactor: unify hummock+memory with hummock+memory-shared
3c87f77 
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao BugenZhao requested review from xxchan, wenym1 and kwannoel May 13, 2025 09:37
@github-actions github-actions bot added the type/refactor Type: Refactoring. label May 13, 2025
@kwannoel
Copy link
Contributor

Any specific reason for this:

After this PR, an embedded compactor will be started by the compute node service. So there's no need to start a compactor service separately any more.

@BugenZhao
Copy link
Member Author
BugenZhao commented May 13, 2025 •
edited
Loading

Any specific reason for this:

After this PR, an embedded compactor will be started by the compute node service. So there's no need to start a compactor service separately any more.

Actually this is the current behavior remaining untouched.

risingwave/src/compute/src/server.rs

Lines 535 to 541 in 3c87f77

/// Checks whether an embedded compactor starts with a compute node.
fn embedded_compactor_enabled(state_store_url: &str, disable_remote_compactor: bool) -> bool {
// Always start an embedded compactor if the state store is in-memory.
state_store_url.starts_with("hummock+memory")
|| state_store_url.starts_with("hummock+disk")
|| disable_remote_compactor
}

So I think previously in playground, we had a functioning embedded compactor and a non-functioning standalone compactor at the same time.

@BugenZhao BugenZhao changed the title refactor: unify hummock+memory with hummock+memory-shared refactor: unify hummock+memory with hummock+memory-shared & fix compactor service in playground May 13, 2025
@BugenZhao
add simple unit test
a20eeb4 
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao
use more for_test
fb52721 
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao
add a memory-isolated-for-test for unit tests
62baee4 
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao
fix unit test
eba4815 
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao BugenZhao enabled auto-merge May 14, 2025 08:51
@BugenZhao BugenZhao added this pull request to the merge queue May 14, 2025
Merged via the queue into main with commit 7617559 May 14, 2025
34 checks passed
@BugenZhao BugenZhao deleted the bz/fellow-emu branch May 14, 2025 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xxchan xxchan xxchan approved these changes

@kwannoel kwannoel kwannoel approved these changes

@wenym1 wenym1 Awaiting requested review from wenym1

Assignees
No one assigned
Labels
ci/main-cron/run-selected ci/run-e2e-single-node-tests ci/run-e2e-standalone-tests ci/run-e2e-test-other-backends type/refactor Type: Refactoring.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BugenZhao @kwannoel @xxchan
0