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: minor cleanups #6239

Merged
merged 2 commits into from May 2, 2023
Merged

orca: minor cleanups #6239

merged 2 commits into from May 2, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented May 1, 2023

  • Move ToReportProto to internal - it was never meant to be exported / part of the ORCA API, but it is tested from another package (orca_test) so it still needs to be exported. (This is also done in orca: allow a ServerMetricsProvider to be passed to the ORCA service and ServerOption #6223, so they will technically conflict but both are ~the same.)
  • Don't error in ToReportProto if there is no load report present. This will prevent error log spam which occurs whenever the ORCA package is imported and a Done callback is set by a picker.
  • Minor log line fix to remove unused %-format item.
  • Downgrade error log to info for ORCA parse error.

RELEASE NOTES: none

@dfawley dfawley added the Type: Internal Cleanup Refactors, etc label May 1, 2023
@dfawley dfawley added this to the 1.56 Release milestone May 1, 2023
@dfawley dfawley requested a review from zasweq May 1, 2023 19:59
orca/internal/internal.go Outdated Show resolved Hide resolved
"errors"
"fmt"

ibackoff "google.golang.org/grpc/internal/backoff"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I saw Easwar comment something about import renaming with no alias/conflict and you replied with to keep it consistent with other packages that conflict with backoff. Does this apply to this?

Copy link
Member Author

Choose a reason for hiding this comment

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

backoff is a top-level package so I wouldn't want to use that name. It looks like we use the more verbose "internalbackoff" in other places. This usage is existing so I'd rather just leave it alone and avoid churn.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I always get confused with balancer/ vs. internal/balancer conflicts. Maybe we should start adopting conventions for internal packages prefixed with i.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be fine with something like that...having consistent names is always good.

orca/internal/internal.go Show resolved Hide resolved
orca/internal/internal.go Show resolved Hide resolved
orca/orca_test.go Show resolved Hide resolved
@zasweq zasweq assigned dfawley and unassigned zasweq May 1, 2023
@dfawley dfawley assigned zasweq and unassigned dfawley May 1, 2023
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM.

@zasweq zasweq assigned dfawley and unassigned zasweq May 1, 2023
@dfawley dfawley merged commit 713bd04 into grpc:master May 2, 2023
11 checks passed
@dfawley dfawley deleted the orcasmall branch May 2, 2023 00:03
tobotg pushed a commit to tobotg/grpc-go that referenced this pull request May 3, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 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.

None yet

2 participants