-
Notifications
You must be signed in to change notification settings - Fork 4.6k
grpclog: refactor to move implementation to grpclog/internal #7465
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
Conversation
|
Things that I'm not thrilled about with this PR:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7465 +/- ##
==========================================
+ Coverage 81.61% 81.97% +0.35%
==========================================
Files 359 360 +1
Lines 27532 27533 +1
==========================================
+ Hits 22470 22569 +99
+ Misses 3835 3778 -57
+ Partials 1227 1186 -41
|
arvindbr8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only nits
grpclog/logger.go
Outdated
| import ( | ||
| "google.golang.org/grpc/grpclog/internal" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| import ( | |
| "google.golang.org/grpc/grpclog/internal" | |
| ) | |
| import "google.golang.org/grpc/grpclog/internal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
scripts/vet.sh
Outdated
| UpdateAddresses is deprecated: | ||
| UpdateSubConnState is deprecated: | ||
| balancer.ErrTransientFailure is deprecated: | ||
| internal.Logger is deprecated: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do something that is specific to that one usage? example: https://github.com/grpc/grpc-go/blob/master/scripts/vet.sh#L125
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. But I also had to leave the current line in there.
- New check with fail if there are any usages of the deprecated
Loggerinterface from anywhere other thanprefix_logger.go. - Once that check passes, this will ensure that the usage in
prefix_logger.godoes not cause vet to fail.
internal/grpclog/prefix_logger.go
Outdated
| if pl == nil { | ||
| return true | ||
| } | ||
| return pl.logger.V(l) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: to stick to the other patterns how about?
| if pl == nil { | |
| return true | |
| } | |
| return pl.logger.V(l) | |
| if pl != nil { | |
| return pl.logger.V(l) | |
| } | |
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
2e35cf3 to
f0ab028
Compare
Idea behind the refactor:
grpclogpackagegrpclog/internalpackageinternal/grpcloggrpclogwill take a dependency ongrpclog/internalinternal/grpclogwill take a dependency ongrpclogAlso introduces a few methods for depth logging to the external
grpclogpackage.RELEASE NOTES: none