orca: cleanup old code, and get grpc package to use new code #5627
Conversation
orca/orca.go
Outdated
| // an import cycle. Hence this roundabout method is used. | ||
| type loadParser struct{} | ||
|
|
||
| func (*loadParser) Parse(md metadata.MD) interface{} { |
There was a problem hiding this comment.
Nit: best practice AIUI is to define this on the value type and avoid pointers.
There was a problem hiding this comment.
go/go-style/decisions#receiver-type
I have read this a few times in the past, but only few things stuck in my head like:
- If the method needs to mutate the receiver, the receiver must be a pointer.
- If the receiver is a struct containing fields that cannot safely be copied, use a pointer receiver.
- When in doubt, use a pointer receiver.
But there seems to be case for this one:
- If the receiver is a "small" array or struct that is naturally a value type with no mutable fields and no pointers, a value receiver is usually the right choice.
orca/orca.go
Outdated
| type loadParser struct{} | ||
|
|
||
| func (*loadParser) Parse(md metadata.MD) interface{} { | ||
| lr, _ := ToLoadReport(md) |
There was a problem hiding this comment.
Added an error log. As far as the caller of this function is concerned (transport), the error does not change anything. So, I did not bother changing the signature of the method to return an error for the caller to deal with it.
The e2e style test that we have for this does create a gRPC client, but simply reads the trailers off the stream instead of installing a custom LB policy. I thought it would be better/easier to add that test once we have client-side support as well. |
| @@ -158,7 +159,7 @@ func (d *picker) Pick(info balancer.PickInfo) (balancer.PickResult, error) { | |||
| } | |||
| d.loadStore.CallFinished(lIDStr, info.Err) | |||
|
|
|||
| load, ok := info.ServerLoad.(*orcapb.OrcaLoadReport) | |||
| load, ok := info.ServerLoad.(*v3orcapb.OrcaLoadReport) | |||
There was a problem hiding this comment.
The comment on info.ServerLoad says "The only supported type now is *orca_v1.LoadReport." Should this be updated?
Fixes #5542
This PR cleans up the old implementation in https://github.com/grpc/grpc-go/tree/master/xds/internal/balancer/orca and gets the
grpcpackage to use the new one (by registering theParserinterface from the new code).RELEASE NOTES: n/a