Fix gRPC listener error message and some minor (subjective) renaming #132545
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| @@ -30,12 +30,12 @@ type nodeRegistrar struct { | |||
| } | |||
|
|
|||
| // startRegistrar returns a running instance. | |||
| func startRegistrar(logger klog.Logger, grpcVerbosity int, interceptors []grpc.UnaryServerInterceptor, streamInterceptors []grpc.StreamServerInterceptor, driverName string, supportedServices []string, socketpath string, pluginRegistrationEndpoint endpoint) (*nodeRegistrar, error) { | |||
| func startRegistrar(logger klog.Logger, grpcVerbosity int, interceptors []grpc.UnaryServerInterceptor, streamInterceptors []grpc.StreamServerInterceptor, driverName, driverEndpointPath string, supportedServices []string, pluginRegistrationEndpoint endpoint) (*nodeRegistrar, error) { | |||
There was a problem hiding this comment.
Moving the parameters looks like an unnecessary change. What's the rationale?
Renaming it is fine, but perhaps let's use "draEndpointPath"? That matches how it is called in the caller.
"driver endpoint" is still vague, a driver has multiple endpoints.
There was a problem hiding this comment.
Haha! Sorry yes that was just a bit of an opinionated preference. I liked the idea that the "driver name" and the "driver path" are together and consecutive, like it is in the registrationServer and PluginInfo structs. But I can understand that is a very superfluous thing to do.
Reverted back the order and renamed as suggested to "draEndpointPath"
There was a problem hiding this comment.
One can also argue that driverEndpointPath and pluginRegistrationEndpoint should be close together... but at this point not making more changes than really necessary trumps slight personal preferences.
There was a problem hiding this comment.
Yep yep. Fair enough :)
4a82906 to
a251fe0
Compare
|
LGTM label has been added. Git tree hash: bffdcf469b4ac4310416865b1b2c0e26e7da5a6e
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauravkghildiyal, pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
- fix: Use correct path in kubeletplugin's gRPC listener error message - chore: Rename socketpath for slightly improved distinction from pluginRegistrationEndpoint
a251fe0 to
3eefb05
Compare
|
Thank you for reviewing! /unhold Squashed the commits as per suggestion. |
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
staging/src/k8s.io/dynamic-resource-allocation/kubeletplugin/nonblockinggrpcserver.goDoes this PR introduce a user-facing change?
/wg device-management