Skip to content
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

orca: cleanup old code, and get grpc package to use new code #5627

Merged
merged 3 commits into from Sep 27, 2022

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Sep 2, 2022

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 grpc package to use the new one (by registering the Parser interface from the new code).

RELEASE NOTES: n/a

@easwars easwars requested a review from dfawley September 2, 2022 21:31
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Sep 2, 2022
@easwars easwars added this to the 1.50 Release milestone Sep 2, 2022
Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there an end-to-end style test for this that creates a gRPC client with a custom LB policy that retrieves ORCA data from the server?

orca/orca.go Outdated
// an import cycle. Hence this roundabout method is used.
type loadParser struct{}

func (*loadParser) Parse(md metadata.MD) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: best practice AIUI is to define this on the value type and avoid pointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dfawley dfawley assigned easwars and unassigned dfawley Sep 7, 2022
@easwars
Copy link
Contributor Author

easwars commented Sep 15, 2022

Was there an end-to-end style test for this that creates a gRPC client with a custom LB policy that retrieves ORCA data from the server?

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.

@easwars easwars assigned dfawley and unassigned easwars Sep 15, 2022
Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small cleanup item, otherwise LGTM

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on info.ServerLoad says "The only supported type now is *orca_v1.LoadReport." Should this be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks.

@easwars easwars merged commit 36e4810 into grpc:master Sep 27, 2022
1 check passed
@easwars easwars deleted the orca_cleanup branch September 27, 2022 19:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

orca: cleanup unused code
2 participants