-
Notifications
You must be signed in to change notification settings - Fork 373
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
Clean up some global vars #2025
base: master
Are you sure you want to change the base?
Conversation
Hi @kevin85421! pinging for a review, was not sure who to ping tbh, I am new here. |
@skonto, thank you for the contribution! Would you mind rebasing with the master branch? I will do my best to review it this week, but I am currently busy with the release process, so I cannot guarantee a timeframe. Maybe @astefanutti @andrewsykim can review this PR if you have time. Thanks! |
cc @rueian would you mind reviewing this PR? |
@@ -1570,3 +1568,25 @@ func sumGPUs(resources map[corev1.ResourceName]resource.Quantity) resource.Quant | |||
|
|||
return totalGPUs | |||
} | |||
|
|||
type clusterConfig struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type clusterConfig struct{} | |
type clusterConfigCtxKey struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about clarifying the purpose of this struct by adding something like CtxKey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should be renamed. Kind of missed it when refactoring.
ctx = ray.WithClusterConfig(ctx, ray.ClusterConfig{ | ||
ForcedClusterUpgrade: config.ForcedClusterUpgrade, | ||
EnableBatchScheduler: config.EnableBatchScheduler, | ||
}) | ||
exitOnError(ray.NewReconciler(ctx, mgr, rayClusterOptions).SetupWithManager(ctx, mgr, config.ReconcileConcurrency), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing state by values into context.Context
is considered an anti-pattern by some people given its unstructured nature.
What about passing the configapi.Configuration
struct (or fields) instead where it's needed, which is rather the idiomatic approach operators take in that case. It'll also avoid the need to declare another configuration struct like ray.ClusterConfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing state by values into context.Context is considered an anti-pattern by some people given its unstructured nature.
Not sure to what extent it is so. I come from the Knative project where we actually use it this way to propagate values a lot. Knative follows the K8s project practices and I have seen this pattern used in that code base too, for example.
What about passing the configapi.Configuration
Ok I can try passing the configuration and see if that is available where we need it and take it from there. In general with any option there are pros and cons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why this may be debated. In essence some people consider contexts should be primarily used for cancellations and propagating termination signals, and only on last resort for storing values. An example of such discussion can be found at https://www.reddit.com/r/golang/comments/10qf73m/context_api_and_best_practices_of_using_it/.
In the particular case of that PR, I still fail to see the benefit of relying on context, over passing the configapi.Configuration
object as an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still fail to see the benefit of relying on context
The generic benefit is not having to change the func signature, allowing passing data down the call chain easier in the future even if they are not a good fit for the config object etc. Of course there are downsides and probably here it is an early optimization. In any case I will adapt the PR to use an argument.
@@ -64,6 +64,8 @@ func main() { | |||
var logFileEncoder string | |||
var logStdoutEncoder string | |||
var configFile string | |||
var EnableBatchScheduler bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the EnableBatchScheduler
and ForcedClusterUpgrade
variables be removed from the raycluster_controller.go
file?
@skonto @astefanutti any updates for this PR? |
Hey sorry for that looong delay I will take a look, fell through the cracks. |
Why are these changes needed?
This moves some global variables to be local and allows to propagate any value via context for future use.
Related issue number
N/A
Checks