[Refactor] Make port name variables consistent and meaningful #1389
Conversation
Add constants to replace string literals in pod.go
Put constants in constant.go
| @@ -125,6 +125,16 @@ const ( | |||
|
|
|||
| // Finalizers for RayJob | |||
| RayJobStopJobFinalizer = "ray.io/rayjob-finalizer" | |||
|
|
|||
| DefaultAddressName = "address" | |||
There was a problem hiding this comment.
We can add a comment to indicate that these are the parameter names for ray start: https://docs.ray.io/en/latest/cluster/cli.html?highlight=dashboard-host#ray-start.
| DefaultMetricsName = "metrics" | ||
| DefaultDashboardAgentListenPortName = "dashboard-agent" | ||
| DefaultServingPortName = "serve" | ||
| DefaultClientPortName = "client" |
There was a problem hiding this comment.
It would be beneficial to maintain consistency in port names. As an example, we could update DefaultMetricsName to DefaultMetricsPortName.
There was a problem hiding this comment.
Thanks for guidance. I've updated the branch.
There was a problem hiding this comment.
However, I have an idea that want to discuss:
Concerning the prefix 'Default' in the name, it might potentially create confusion for developers. In this context, 'Default' implies that the value can be modified and configured, which may not be suitable. Moreover, it could be confused for new contributors who are trying to follow along.
There was a problem hiding this comment.
Good point. Maybe we can remove "Default" here.
kevin85421
changed the title
…oject#1389) Make port name variables consistent and meaningful
Why are these changes needed?
According to @akanso's style suggestion, use constants to replace string literals and put them in separate file.
Therefore, if they change, we only need to modify them once.
#328 (comment)
Related issue number
#337
Checks