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

refactor(streaming): unify and refine rate_limit related type #19857

Open
MrCroxx opened this issue Dec 18, 2024 · 7 comments
Open

refactor(streaming): unify and refine rate_limit related type #19857

MrCroxx opened this issue Dec 18, 2024 · 7 comments

Comments

@MrCroxx
Copy link
Contributor

MrCroxx commented Dec 18, 2024

Background

Currently, we are using two types to present "rate_limit".

In .proto files, all rate limit values are optional uint32:

  • None: infinite
  • 0: pause
  • other value v: rate limit at v

However, when we are parsing the rate limit with SQL, we are using i32:

  • -1 (and other negative value for compatibility): infinite
  • 0: pause
  • other positive value v: rate limit at v

Although it looks wired, it works fine for now.

But things are getting worse when we want to introduce some new rate limit policy. It is too tricky to use more conventional values, especially with u32 type. (e.g. To introduce the adaptive rate limiter for backfill, #19743 )

Design

Use a new message Throttle to unify and refine all rate limits.

e.g.

enum ThrottleKind {
	THROTTLE_KIND_ADAPTIVE = 1;
	THROTTLE_KIND_INFINITE = 2
	THROTTLE_KIND_FIXED = 3;
}

message Throttle {
	ThrottleKind kind = 1;
	// Only valid when `kind` is `FIXED`.
	uint32 value = 2;
}

Future Optimizations

No response

Discussions

No response

Q&A

No response

@MrCroxx MrCroxx self-assigned this Dec 18, 2024
@github-actions github-actions bot added this to the release-2.2 milestone Dec 18, 2024
@MrCroxx MrCroxx assigned MrCroxx and unassigned MrCroxx Dec 18, 2024
@MrCroxx
Copy link
Contributor Author

MrCroxx commented Dec 18, 2024

Any comments? cc @hzxa21 @StrikeW @kwannoel @tabVersion

@tabVersion
Copy link
Contributor

at least it should not be THROTTLE_KIND_INFINiTE (THROTTLE_KIND_INFINITE) 🤣

@MrCroxx
Copy link
Contributor Author

MrCroxx commented Dec 18, 2024

at least it should not be THROTTLE_KIND_INFINiTE (THROTTLE_KIND_INFINITE) 🤣

Fixed. 🫠

@tabVersion
Copy link
Contributor

proposing

enum ThrottleKind {
	THROTTLE_KIND_ADAPTIVE = 1;
	THROTTLE_KIND_INFINITE = 2
	THROTTLE_KIND_FIXED = 3;
++  THROTTLE_KIND_PAUSE = 4; // more distinguishable than contained by FIXED
}

message Throttle {
	ThrottleKind kind = 1;
	// Only valid when `kind` is `FIXED`.
	uint32 value = 2;
}

@kwannoel
Copy link
Contributor

How about THROTTLE_KIND_DISABLED instead of infinite.

@MrCroxx
Copy link
Contributor Author

MrCroxx commented Dec 18, 2024

Proposings on naming accepted.

Can I assume this refactor works for you?

We need to keep both fields for a few versions. Because the streaming plan is stored as proto in meta

@kwannoel
Copy link
Contributor

Proposings on naming accepted.

Can I assume this refactor works for you?

We need to keep both fields for a few versions. Because the streaming plan is stored as proto in meta

LGTM, thanks for the initiative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants