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(SBC): Provision proto #20452

Merged
merged 9 commits into from
Feb 18, 2025
Merged

feat(SBC): Provision proto #20452

merged 9 commits into from
Feb 18, 2025

Conversation

CAJan93
Copy link
Contributor

@CAJan93 CAJan93 commented Feb 11, 2025

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

Communication between kernel and the Serverless Backfill Controller (SBC). Proto definitions should be in this repo and not in the SBC repo.

Equivalent on SBC side: https://github.com/risingwavelabs/serverless-backfill-controller/pull/76

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.
  • My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

  • My PR needs documentation updates.
Release note

@CAJan93 CAJan93 changed the title provision proto feat(SBC): Provision proto Feb 11, 2025
@CAJan93 CAJan93 marked this pull request as ready for review February 11, 2025 09:39
@CAJan93 CAJan93 self-assigned this Feb 11, 2025
@CAJan93 CAJan93 requested review from shanicky and arkbriar February 11, 2025 09:40
Copy link
Contributor

@arkbriar arkbriar left a comment

Choose a reason for hiding this comment

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

string label = 1;
}

service NodeGroupController {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we unify the term, such as ResourceGroupController?

@shanicky PTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we plan to provision something other than node groups? If not we can also be precise and use NodeGroupController.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with both. Resource group is what's used in the kernel, while node group is from the operator. They do have a 1-1 mapping.

@arkbriar arkbriar requested review from hzxa21 and fuyufjh February 12, 2025 10:09
@CAJan93
Copy link
Contributor Author

CAJan93 commented Feb 12, 2025

I will add the Proto-related changes in https://github.com/risingwavelabs/serverless-backfill-controller/pull/81

Copy link
Contributor

@shanicky shanicky left a comment

Choose a reason for hiding this comment

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

LGTM; it aligns with the RFC, but please keep the SBC-related code as clean as possible in the future.

syntax = "proto3";

// Provision a node via the serverless backfill controller.
package sbc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use a full name? Future colleagues might not immediately associate "sbc" with anything.

@CAJan93 CAJan93 enabled auto-merge February 17, 2025 13:30
@CAJan93 CAJan93 added this pull request to the merge queue Feb 18, 2025
Merged via the queue into main with commit 7aabcad Feb 18, 2025
11 of 12 checks passed
@CAJan93 CAJan93 deleted the cajan93/sbc-proto branch February 18, 2025 08:03
@CAJan93 CAJan93 restored the cajan93/sbc-proto branch February 18, 2025 08:27
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.

3 participants