Skip to content

Conversation

@cspwizard
Copy link

netstd: Log errors in processor via standard logger

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • 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.

@Jens-G Jens-G self-requested a review November 21, 2020 14:07
@Jens-G Jens-G closed this in 1506661 Nov 21, 2020
@Jens-G
Copy link
Member

Jens-G commented Nov 21, 2020

I made the logger optional to a) not break existing code (including the Test suite) and b) not to force people into using it.

@cspwizard
Copy link
Author

cspwizard commented Nov 22, 2020

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.

@Jens-G
Copy link
Member

Jens-G commented Nov 22, 2020

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.

@cspwizard
Copy link
Author

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

@Jens-G
Copy link
Member

Jens-G commented Nov 22, 2020

I already merged you PR, in case you did'nt notice. Further PRs welcome of course.

@cspwizard
Copy link
Author

Great, thanks

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