You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/11/21 10:23:01 UTC
[GitHub] [thrift] cspwizard opened a new pull request #2284: netstd: Log errors in processor via standard logger
cspwizard opened a new pull request #2284:
URL: https://github.com/apache/thrift/pull/2284
netstd: Log errors in processor via standard logger
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cspwizard edited a comment on pull request #2284: netstd: Log errors in processor via standard logger
Posted by GitBox <gi...@apache.org>.
cspwizard edited a comment on pull request #2284:
URL: https://github.com/apache/thrift/pull/2284#issuecomment-731711515
I may miss the idea, but how is it optional when there is an output directly to console? When we use docker we are already printing logs to console, in specific formats.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cspwizard commented on pull request #2284: netstd: Log errors in processor via standard logger
Posted by GitBox <gi...@apache.org>.
cspwizard commented on pull request #2284:
URL: https://github.com/apache/thrift/pull/2284#issuecomment-731731282
Great, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2284: netstd: Log errors in processor via standard logger
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2284:
URL: https://github.com/apache/thrift/pull/2284#issuecomment-731729231
Main reason for the change was that you added the logger argument in a way that broke existing code. You could have easily recognised this yourself just by trying to build the code we have. Since logging is a non-mission critical task to Thrift the very obvious solution was to make the argument option by adding `= default` and of course take care of that case when the logger is used. That way you are free to provide a logger but are not forced to do it, in which case it simply behaves as before. I'd consider this as the optimum solution. Whether or not someone is using docker or not is of no relevance here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cspwizard commented on pull request #2284: netstd: Log errors in processor via standard logger
Posted by GitBox <gi...@apache.org>.
cspwizard commented on pull request #2284:
URL: https://github.com/apache/thrift/pull/2284#issuecomment-731711515
I may miss the idea, but how is it optional when there is an output directly to console?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cspwizard commented on pull request #2284: netstd: Log errors in processor via standard logger
Posted by GitBox <gi...@apache.org>.
cspwizard commented on pull request #2284:
URL: https://github.com/apache/thrift/pull/2284#issuecomment-731729816
Ah, ok. I'll change the code.
My point was that the logger/console in current implementation is not optional in any way. It just prints unformatted text to a console and that brings some problems in use
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2284: netstd: Log errors in processor via standard logger
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2284:
URL: https://github.com/apache/thrift/pull/2284#issuecomment-731730593
I already merged you PR, in case you did'nt notice. Further PRs welcome of course.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G closed pull request #2284: netstd: Log errors in processor via standard logger
Posted by GitBox <gi...@apache.org>.
Jens-G closed pull request #2284:
URL: https://github.com/apache/thrift/pull/2284
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on pull request #2284: netstd: Log errors in processor via standard logger
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2284:
URL: https://github.com/apache/thrift/pull/2284#issuecomment-731587159
I made the logger optional to a) not break existing code (including the Test suite) and b) not to force people into using it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org