Skip to content

KAFKA-2633: Default logging from tools to Stderr#296

Closed
granthenke wants to merge 1 commit intoapache:trunkfrom
granthenke:tools-log4j
Closed

KAFKA-2633: Default logging from tools to Stderr#296
granthenke wants to merge 1 commit intoapache:trunkfrom
granthenke:tools-log4j

Conversation

@granthenke
Copy link
Copy Markdown
Member

No description provided.

@gwenshap
Copy link
Copy Markdown
Contributor

LGTM. Not waiting for Jenkins since we don't have any relevant tests.

@asfgit asfgit closed this in fe4818e Oct 12, 2015
@ewencp
Copy link
Copy Markdown
Contributor

ewencp commented Oct 13, 2015

Just going to throw my 2 compatibility czar 2¢ in here -- this is clearly the right thing to do long term, but potentially breaks any scripts that have been built on the existing tools with loggers sending output to stdout. tools-log4j.properties is pretty far-reaching. Are you comfortable that every log message from any of our tools that doesn't override the log4j settings is non-critical?

One simple example that I imagine could cause breakage for a lot of people: from a quick scan, the partition reassignment tool prints lots of useful info to stdout.

@granthenke
Copy link
Copy Markdown
Member Author

At a quick glance, the partition reassignment tool is using println() to print out most of its information. This change will not effect that. This change only effects calls to the logger. It also does not prevent the information from being printed, but allows a user to redirect it in case they do not want it to effect the output (with 2>). I don't suspect a user is writing their scripts to depend on logged output, however if they are they should still be able to. I feel most would be relieved to have a good default behavior.

Since we are just before a major release, I think now is a good time to make a change like this. Especially when the public cli has been broken already. Here are some some notable potentially backwards incompatible changes (not an exhaustive list, sorted by potential impact):

  • KAFKA-2205 - No longer able to alter topic config from the topics command
  • KAFKA-2514 - default JVM options changed for everything that uses kafka-run-class.sh
  • KAFKA-2198 - topics commands now exits with non-zero exit code on failure
  • KAFKA-2337 - added error and warning output in topic command to avoid topic colision

@gwenshap
Copy link
Copy Markdown
Contributor

From my perspective, the current situation (errors written to stdout) is a very annoying bug which should be fixed. I don't think we want to maintain buggy behavior as part of the compatibility guarantees.

I think separating output from errors is in the best interests of everyone who runs tools with automation. For example, if your script parses output of ConsumerOffset tool, being able to route errors somewhere else will be very beneficial.

Same for being able to bump log level of tools while maintaining readable output.

@granthenke
Copy link
Copy Markdown
Member Author

I am happy to send a docs patch listing a potentially breaking change. It can be a new jira or a follow up patch. We should likely enumerate the breaking changes I listed in my PR comment somewhere as well.

Obligatory XKCD comic: https://xkcd.com/1172/

@ewencp
Copy link
Copy Markdown
Contributor

ewencp commented Oct 13, 2015

@gwenshap I agree, we obviously have to be able to make progress on improving the CLI tools. This was just a particularly broad change since it affects every tool that does any logging via log4j, although as @granthenke points out that may not be that big a change since a lot of the tools just write directly to stdout.

@granthenke Pointing out those other potential compatibility issues is very valuable, but 5 wrongs don't make a right :) Just because we may have missed other compatibility issues isn't an argument that adding more is ok.

I'm fine with this change as long as we're convinced it doesn't affect anything that's likely to be critical. I just like to raise the compatibility issue whenever I notice it so we continue to pay attention to it. CLI is an especially difficult area because it's harder to warn users that may be using the CLI in automated ways. Unlike the Java code where you can add @Deprecated and they should see the warnings, the CLI changes can't be "staged" as easily.

I think it'd be great to add a changelog for compatibility breaking changes. Kafka's current "release notes" (i.e. https://archive.apache.org/dist/kafka/0.8.2.0/RELEASE_NOTES.html) is not all that helpful to users.

@granthenke
Copy link
Copy Markdown
Member Author

@ewencp completely agree about 5 wrongs making a right. What I was suggesting was that if we have made script breaking changes for 0.9.0, (I assumed they were know to be potentially breaking) then lets do this one now too.

I will open a jira send a PR adding these breaking changes to the release notes.

@granthenke
Copy link
Copy Markdown
Member Author

@ewencp Since KAFKA-2205 has a relative big breaking change...do we need to open a jira to fix that? I can send a PR fixing that today too.

We can add the ability to change topic configs back, and log that its deprecated.

@gwenshap
Copy link
Copy Markdown
Contributor

AFAIK, we don't write release notes. They are generated automatically from JIRA before a release. @junrao can correct me on this.

I suggest, since 0.9.0.0 will be a big upgrade, to open an umbrella JIRA for missing docs. IMO we need a new doc section called "Upgrade Guide" with both instructions on how to upgrade and long list of behaviors that changed (ISR max lag as a popular example). Responsible admins will read the guide before upgrading and modify scripts accordingly.
New features should be added to existing docs, although Security probably deserves its own section too.

@granthenke
Copy link
Copy Markdown
Member Author

@gwenshap there is an upgrade section here. Perhaps we just need to fill that out more.

@granthenke granthenke deleted the tools-log4j branch November 17, 2015 17:06
efeg pushed a commit to efeg/kafka that referenced this pull request Jan 29, 2020
davide-armand pushed a commit to aiven/kafka that referenced this pull request Dec 1, 2025
Merger is getting stuck for unknown reason using PG control plane.
Let's disable it until it is fixed.
jeqo added a commit to aiven/kafka that referenced this pull request Jan 16, 2026
Merger is getting stuck for unknown reason using PG control plane.
Let's disable it until it is fixed.
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.

3 participants