[history server]Add cmdline flags to make qps and burst configurable#4821
[history server]Add cmdline flags to make qps and burst configurable#4821KunWuLuan wants to merge 2 commits into
Conversation
| type ClientManagerConfig struct { | ||
| Kubeconfigs string | ||
| UseKubernetesProxy bool | ||
| QPS float32 | ||
| Burst int | ||
| } | ||
|
|
||
| func NewClientManager(cfg ClientManagerConfig) (*ClientManager, error) { | ||
| kubeconfigs := cfg.Kubeconfigs | ||
| useKubernetesProxy := cfg.UseKubernetesProxy | ||
| qps := cfg.QPS | ||
| burst := cfg.Burst |
There was a problem hiding this comment.
why we need Kubeconfigs and UseKubernetesProxy?
There was a problem hiding this comment.
UseKubernetesProxy is imported by https://github.com/ray-project/kuberay/pull/4404/changes.
Kubeconfigs is used for attaching live dashboards
Future-Outlier
left a comment
Future-Outlier
left a comment
There was a problem hiding this comment.
Can you update the PR title by adding history server as a string prefix?
KunWuLuan
changed the title
| flag.Float64Var(&qps, "kube-api-qps", 0, "QPS to use for Kubernetes API client (default 50)") | ||
| flag.IntVar(&burst, "kube-api-burst", 0, "Burst to use for Kubernetes API client (default 100)") |
There was a problem hiding this comment.
| flag.Float64Var(&qps, "kube-api-qps", 0, "QPS to use for Kubernetes API client (default 50)") | |
| flag.IntVar(&burst, "kube-api-burst", 0, "Burst to use for Kubernetes API client (default 100)") | |
| flag.Float64Var(&qps, "kube-api-qps", float64(configapi.DefaultQPS), | |
| "The QPS value for the client communicating with the Kubernetes API server.") | |
| flag.IntVar(&burst, "kube-api-burst", configapi.DefaultBurst, | |
| "The maximum burst for throttling requests from this client to the Kubernetes API server.") |
Can we align with ray-operator pattern?
Lines 105 to 106 in 76c7d7a
Unless there's a specific reason to be more conservative, I would also suggest sticking with ray-operator defaults.
| if qps <= 0 { | ||
| qps = 50 | ||
| } | ||
| if burst <= 0 { | ||
| burst = 100 | ||
| } |
There was a problem hiding this comment.
Once we correctly handle defaulting in flags, we can remove this fallback in the constructor.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Signed-off-by: KunWuLuan <kunwuluan@gmail.com>
KunWuLuan
force-pushed
the
master
branch
2 times, most recently
from
646710e to
25308b3
Compare
…ve flag descriptions Address PR review comments: use ray-operator aligned defaults (QPS=100, Burst=200) directly in flag definitions and remove redundant fallback logic in NewClientManager. Signed-off-by: KunWuLuan <kunwuluan@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aa8e9a3. Configure here.
|
|
||
| "github.com/sirupsen/logrus" | ||
|
|
||
| configapi "github.com/ray-project/kuberay/ray-operator/apis/config/v1alpha1" |
There was a problem hiding this comment.
Heavy dependency import for two trivial constants
Medium Severity
Importing configapi "github.com/ray-project/kuberay/ray-operator/apis/config/v1alpha1" solely for two constant values (DefaultQPS = float64(100) and DefaultBurst = 200) pulls in heavy transitive dependencies. That package's config_utils.go imports batch scheduler packages (volcano, scheduler-plugins, kai-scheduler, yunikorn), adding sigs.k8s.io/scheduler-plugins and volcano.sh/apis as new indirect dependencies to the historyserver binary. Defining these two trivial constants locally would avoid this dependency bloat entirely.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit aa8e9a3. Configure here.
3 participants
Why are these changes needed?
Related issue number
fix #4483
Checks