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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions orca/orca.go
Expand Up @@ -34,6 +34,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/balancerload"
"google.golang.org/grpc/metadata"
"google.golang.org/protobuf/proto"

Expand Down Expand Up @@ -162,3 +163,21 @@ func ToLoadReport(md metadata.MD) (*v3orcapb.OrcaLoadReport, error) {
}
return ret, nil
}

// loadParser implements the Parser interface defined in `internal/balancerload`
// package. This interface is used by the client stream to parse load reports
// sent by the server in trailer metadata. The parsed loads are then sent to
// balancers via balancer.DoneInfo.
//
// The grpc package cannot directly call orca.ToLoadReport() as that would cause
// an import cycle. Hence this roundabout method is used.
type loadParser struct{}

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

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.

lr, _ := ToLoadReport(md)
Copy link
Member

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.

return lr
}

func init() {
balancerload.SetParser(&loadParser{})
}
5 changes: 3 additions & 2 deletions xds/internal/balancer/clusterimpl/picker.go
Expand Up @@ -19,14 +19,15 @@
package clusterimpl

import (
orcapb "github.com/cncf/xds/go/xds/data/orca/v3"
"google.golang.org/grpc/balancer"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/internal/wrr"
"google.golang.org/grpc/status"
"google.golang.org/grpc/xds/internal/xdsclient"
"google.golang.org/grpc/xds/internal/xdsclient/load"

v3orcapb "github.com/cncf/xds/go/xds/data/orca/v3"
)

// NewRandomWRR is used when calculating drops. It's exported so that tests can
Expand Down Expand Up @@ -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
Member

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.

if !ok {
return
}
Expand Down
84 changes: 0 additions & 84 deletions xds/internal/balancer/orca/orca.go

This file was deleted.

96 changes: 0 additions & 96 deletions xds/internal/balancer/orca/orca_test.go

This file was deleted.