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

Replace glog with grpclog #118

Merged
merged 1 commit into from
Mar 1, 2016

Conversation

cuongdo
Copy link

@cuongdo cuongdo commented Mar 1, 2016

This allows projects with forked versions of glog (e.g. CockroachDB) to use grpc-gateway without a flag collision (runtime panic) on the glog flags such as -logtostderr. This also allows integration with any logging systems that have been integrated with grpclog.

Note that I only replaced glog in the runtime package and in the files generated by protoc-gen-grpc-gateway. The use of glog elsewhere in this repo doesn't seem to affect ability to use grpc-gateway in projects with forked glog versions.

Fixes #92.

Review on Reviewable

@@ -82,7 +82,7 @@ func (s *ServeMux) ServeHTTP(w http.ResponseWriter, r *http.Request) {
for _, h := range s.handlers[r.Method] {
pathParams, err := h.pat.Match(components, verb)
if err != nil {
glog.V(3).Infof("path mismatch: %q to %q", path, h.pat)
grpclog.Printf("path mismatch: %q to %q", path, h.pat)
Copy link
Member

Choose a reason for hiding this comment

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

This log is too verbose to always record. Let's remove it.

@yugui
Copy link
Member

yugui commented Mar 1, 2016

@cuongdo Thank you.
I have added some comments on logs that can happen in normal flow. Could you remove the logs because they are too verbose to show in normal flow?

@cuongdo
Copy link
Author

cuongdo commented Mar 1, 2016

Thank you for your fast response. I've made the changes you suggested.


Review status: 0 of 10 files reviewed at latest revision, 5 unresolved discussions.


runtime/mux.go, line 85 [r1] (raw file):
Done.


runtime/pattern.go, line 147 [r1] (raw file):
Done.


runtime/pattern.go, line 163 [r1] (raw file):
Done.


runtime/pattern.go, line 169 [r1] (raw file):
Done.


runtime/pattern.go, line 194 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

This allows projects with forked versions of glog (e.g. CockroachDB) to
use grpc-gateway without a flag collision on the glog flags.
@cuongdo
Copy link
Author

cuongdo commented Mar 1, 2016

Also, I've removed the "Pattern successfully built" log message at the end of NewPattern, since that appears often as well.


Review status: 0 of 10 files reviewed at latest revision, 5 unresolved discussions.


Comments from the review on Reviewable.io

@yugui
Copy link
Member

yugui commented Mar 1, 2016

LGTM.

Thank you for your contribution.

yugui added a commit that referenced this pull request Mar 1, 2016
@yugui yugui merged commit c5dce9e into grpc-ecosystem:master Mar 1, 2016
@cuongdo
Copy link
Author

cuongdo commented Mar 1, 2016

Great. Thanks for your very fast response!

On Tue, Mar 1, 2016 at 6:37 PM Yuki Yugui Sonoda [email protected]
wrote:

Merged #118 #118.


Reply to this email directly or view it on GitHub
#118 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants