Skip to content
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.

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

feat: try to remove parallel unit mapping in frontend #16205

Merged
merged 11 commits into from
Apr 24, 2024
Merged

Conversation

shanicky
Copy link
Contributor

@shanicky shanicky commented Apr 8, 2024

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

What's changed and what's your intention?

This PR attempts to remove the dependency on parallel unit mapping in the frontend and aims to minimize the occurrence of parallel units in the frontend as much as possible.

By replacing ParallelUnit with Worker, we can forcefully substitute ParallelUnitMapping with WorkerMapping. Essentially, the frontend locates the parallel unit through these vnodes, and then maps the parallel unit to the worker. Therefore, we can directly provide worker mapping.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

@shanicky shanicky force-pushed the peng/fe-remove-pu branch 2 times, most recently from 75c32cb to 738db9a Compare April 10, 2024 10:10
@shanicky shanicky marked this pull request as ready for review April 10, 2024 10:28
@shanicky shanicky changed the title feat: try to remove parallel unit mapping in frontend [wip]feat: try to remove parallel unit mapping in frontend Apr 10, 2024
@shanicky shanicky force-pushed the peng/fe-remove-pu branch 2 times, most recently from cce0081 to 55bdf78 Compare April 11, 2024 07:45
@shanicky shanicky changed the title [wip]feat: try to remove parallel unit mapping in frontend feat: try to remove parallel unit mapping in frontend Apr 11, 2024
@shanicky shanicky force-pushed the peng/fe-remove-pu branch from 55bdf78 to be9203d Compare April 11, 2024 09:09
@shanicky shanicky mentioned this pull request Apr 16, 2024
6 tasks
@shanicky shanicky force-pushed the peng/fe-remove-pu branch 2 times, most recently from 480a376 to ff0cccb Compare April 16, 2024 17:45
Refactor fragment mappings and cleanup code

Refactor worker_node, enhance vnode logic, update imports & mapping

Refactor: Disable `FragmentParallelUnitMapping` in protobuf and Rust code

Refactor/gen: Improve formatting in `risingwave_common` tests

Removed proto comments; updated Rust imports and tests.

Remove `FragmentParallelUnitMapping` import and extra lines in `observer_manager.rs`

Refactor FragManager, cleanup proto fields & comments

Remove commented-out alternative vnode placement code

Refactor: Clean up imports in fragment and streaming_job

Refactor worker mapping in join operation

Refactor fields() output in db lib for brevity

Refine comments on migration and worker ID terms
@shanicky shanicky changed the base branch from main to peng/fe-remove-pu-2 April 16, 2024 18:08
@shanicky shanicky changed the base branch from peng/fe-remove-pu-2 to main April 16, 2024 18:08
@shanicky shanicky force-pushed the peng/fe-remove-pu branch from ff0cccb to 353adec Compare April 16, 2024 18:10
@shanicky shanicky force-pushed the peng/fe-remove-pu branch from 353adec to 724d037 Compare April 16, 2024 18:14
@shanicky shanicky requested a review from yezizp2012 April 18, 2024 07:03
@shanicky shanicky requested review from chenzl25 and zwang28 April 18, 2024 07:04
Copy link
Contributor

@chenzl25 chenzl25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this PR only changes the frontend part and on the meta side, it still maintains the vnode to parallel unit mapping.

@shanicky
Copy link
Contributor Author

IIUC, this PR only changes the frontend part and on the meta side, it still maintains the vnode to parallel unit mapping.

Yes, this PR simply removes the frontend's dependency on the parallel unit mapping. To fully eliminate the parallel unit, changes need to be made in both the scheduler and the scaling system, which will be addressed in updates #16167 and #16324.

Copy link
Contributor

@zwang28 zwang28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@chenzl25 chenzl25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@yezizp2012 yezizp2012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM! thx for this PR.
We'd better reserve the fields in proto to avoid backward compatibility, since all the protos are possibly used in cloud side as well.

src/meta/src/controller/catalog.rs Outdated Show resolved Hide resolved
let fragment_mappings = get_fragment_mappings_by_jobs(&txn, streaming_jobs.clone()).await?;

let fragment_mappings = fragment_mappings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we provide a helper function to wrap it, since this part will be used as long as the fragment mappings is transformed?

@@ -274,8 +278,30 @@ impl CatalogController {
.into_tuple()
.all(&txn)
.await?;

let parallel_unit_to_worker = Self::get_parallel_unit_to_worker_map(&txn).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, we might need to cache it as well when we dully deprecate parallel units and maintain the fragment mapping in memory.

@shanicky shanicky enabled auto-merge April 24, 2024 07:29
@shanicky shanicky added this pull request to the merge queue Apr 24, 2024
Merged via the queue into main with commit 4a8b1e5 Apr 24, 2024
31 of 32 checks passed
@shanicky shanicky deleted the peng/fe-remove-pu branch April 24, 2024 08:15
@xxchan xxchan mentioned this pull request Apr 25, 2024
shanicky added a commit that referenced this pull request Apr 28, 2024
@shanicky shanicky restored the peng/fe-remove-pu branch May 6, 2024 09:10
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.

4 participants