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

AVRO-2875: Add logging to avro-tools #925

Merged
merged 2 commits into from Nov 12, 2020

Conversation

RyanSkraba
Copy link
Contributor

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@probot-autolabeler probot-autolabeler bot added build Java Pull Requests for Java binding labels Jul 2, 2020
Copy link
Contributor

@busbey busbey left a comment

Choose a reason for hiding this comment

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

good: the LICENSE for the avro-tools jar already has a section for slf4j due to our use of slf4j-api and it is generic enough to also cover slf4j-simple.

needs to change: avro-tools embeds a log4j.properties file that defines the logging output. we should remove that file since we're not going to use log4j with this update. if we can make the format match that would be ideal. if it doesn't then we need to release note the difference.

@busbey
Copy link
Contributor

busbey commented Jul 2, 2020

occurred to me: do we need one of the other slf4j bridge jars in order to e.g. get the logging from the hadoop libraries we are including in the tools jar?

@RyanSkraba
Copy link
Contributor Author

@busbey Good point! It looks like both points can be resolved by restoring the slf4j-log4j12 binding that used to be brought in by Hadoop 2.7.7 (still used by Hadoop 3.x).

With this change both avro-tools-1.9.2.jar and avro-tools-1.11.0-SNAPSHOT.jar will produce the following Hadoop warning:

20/07/23 15:33:43 WARN util.NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable

(Missing in 1.10.0)

@RyanSkraba
Copy link
Contributor Author

@busbey Hello! Can you PTAL? This should restore the pre-1.10.0 behaviour for logging in avro-tools.

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM

@iemejia
Copy link
Member

iemejia commented Nov 12, 2020

I am merging this because it makes sense and fixes the 'logging regression', in any case don't hesitate to tell if you prefer this done other way @busbey and we will fix it.

@iemejia iemejia merged commit b39848b into apache:master Nov 12, 2020
RyanSkraba added a commit to RyanSkraba/avro that referenced this pull request Nov 12, 2020
* AVRO-2875: Add logging to avro-tools

* Use slf4j-log4j12 instead of slf4j-simple
@RyanSkraba
Copy link
Contributor Author

Cherry-picked to branch-1.10.

@RyanSkraba RyanSkraba deleted the AVRO-2875-avro-tool-logger branch November 12, 2020 16:48
gabriel-tincu pushed a commit to aiven/avro that referenced this pull request Nov 18, 2020
* AVRO-2875: Add logging to avro-tools

* Use slf4j-log4j12 instead of slf4j-simple
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Java Pull Requests for Java binding
Projects
None yet
3 participants