[Limiter] Expose rule book as sys_rule SQL table #4692
[Limiter] Expose rule book as sys_rule SQL table #4692tillrohrmann merged 2 commits intorestatedev:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
tillrohrmann
force-pushed
the
issues/4655-5
branch
from
ec23db7 to
c6fc2f4
Compare
|
@codex review |
|
Shouldn't the table name be plural? |
| /// rule applies to. Examples: `"*"`, `"scope1/*"`, `"scope1/foo/bar"`. | ||
| #[cfg_attr(feature = "schema", schema(value_type = String))] | ||
| #[serde_as(as = "serde_with::DisplayFromStr")] | ||
| pub pattern: RulePattern<ReString>, |
There was a problem hiding this comment.
So far we didn't find a better way for specifying it.
| /// Free-form description shown in the rule book; not consulted at | ||
| /// runtime. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub reason: Option<String>, |
There was a problem hiding this comment.
Awesome idea, maybe call it description?
There was a problem hiding this comment.
The credit goes to Ahmed :-) description is indeed a better name. Will update it.
Yes, of course. I will update it. |
tillrohrmann
force-pushed
the
issues/4655-5
branch
2 times, most recently
from
dd919fa to
fbb42ba
Compare
tillrohrmann
requested review from
AhmedSoliman and
slinkydeveloper
and removed request for
AhmedSoliman
tillrohrmann
force-pushed
the
issues/4655-5
branch
from
fbb42ba to
21aeb04
Compare
| let book = client | ||
| .get::<RuleBook>(RULE_BOOK_KEY.clone()) | ||
| .await | ||
| .map_err(|e| DataFusionError::External(Box::new(e)))? | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Do we have other cases where we perform an inline read from the metadata store when querying DF? Given that most of the time this will be the same value, do we want to rely on the local cache for reads and only fallback to direct metadata reads if we don't have a cache?
There was a problem hiding this comment.
True. Reading from the cache should be good enough if it's not empty.
AhmedSoliman
left a comment
AhmedSoliman
left a comment
There was a problem hiding this comment.
Looks good, my only concern is the inline metadata-store reads and the impact the UI might create on load on metadata store.
tillrohrmann
force-pushed
the
issues/4655-5
branch
2 times, most recently
from
43c14a3 to
112eb8e
Compare
|
The failing test case should be unrelated. |
tillrohrmann
force-pushed
the
issues/4655-5
branch
from
112eb8e to
ee61ad3
Compare
Adds a non-partitioned sys_rules DataFusion table that projects the cluster-global rule book on demand. The scanner reads RULE_BOOK_KEY straight from the metadata store on each query so the table is available on admin-only and worker nodes alike. As a side effect, on nodes with a local RuleBookCache the freshly fetched book is pushed through the existing RuleBookObserver, advancing per-PP visibility without waiting for the next poll tick. The RuleBookObserver type alias moves from restate-admin to restate-limiter so restate-storage-query-datafusion can use it without depending on admin. Additionally we have aligned the limit and usage values to be u32 as it is unlikely that users will ever set limit values > u32::MAX. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tillrohrmann
force-pushed
the
issues/4655-5
branch
from
ee61ad3 to
efb6271
Compare
4 participants
Adds a non-partitioned sys_rules DataFusion table that projects the
cluster-global rule book on demand. The scanner reads RULE_BOOK_KEY
straight from the metadata store on each query so the table is
available on admin-only and worker nodes alike. As a side effect, on
nodes with a local RuleBookCache the freshly fetched book is pushed
through the existing RuleBookObserver, advancing per-PP visibility
without waiting for the next poll tick.
The RuleBookObserver type alias moves from restate-admin to
restate-limiter so restate-storage-query-datafusion can use it without
depending on admin.
This PR is based on #4691.