Skip to content

KAFKA-13598: set log4j appender to default acks#11767

Closed
vvcephei wants to merge 1 commit intotrunkfrom
fix-log4j-appender
Closed

KAFKA-13598: set log4j appender to default acks#11767
vvcephei wants to merge 1 commit intotrunkfrom
fix-log4j-appender

Conversation

@vvcephei
Copy link
Copy Markdown
Contributor

After #11691, the
KafkaLog4JAppender causes the process to crash due
to the mismatch between the new produce default
idempotent mode and the appender's overridden
acks=1 property.

  • Removes the acks=1 override, so the appender falls
    back on the producer's default acks config.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@vvcephei
Copy link
Copy Markdown
Contributor Author

This issue surfaced in Confluent's ksqlDB. Once ksqlDB started to build against Kafka post-#11691, it crashes with:

Exception in thread "main" java.lang.ExceptionInInitializerError
	at org.slf4j.impl.Log4jLoggerFactory.<init>(Log4jLoggerFactory.java:66)
	at org.slf4j.impl.StaticLoggerBinder.<init>(StaticLoggerBinder.java:72)
	at org.slf4j.impl.StaticLoggerBinder.<clinit>(StaticLoggerBinder.java:45)
	at org.slf4j.LoggerFactory.bind(LoggerFactory.java:150)
	at org.slf4j.LoggerFactory.performInitialization(LoggerFactory.java:124)
	at org.slf4j.LoggerFactory.getILoggerFactory(LoggerFactory.java:417)
	at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:362)
	at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:388)
	at io.confluent.ksql.rest.server.KsqlServerMain.<clinit>(KsqlServerMain.java:39)
Caused by: org.apache.kafka.common.config.ConfigException: Must set acks to all in order to use the idempotent producer. Otherwise we cannot guarantee idempotence.
	at org.apache.kafka.clients.producer.ProducerConfig.postProcessAndValidateIdempotenceConfigs(ProducerConfig.java:475)
	at org.apache.kafka.clients.producer.ProducerConfig.postProcessParsedConfig(ProducerConfig.java:443)
	at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:114)
	at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:133)
	at org.apache.kafka.clients.producer.ProducerConfig.<init>(ProducerConfig.java:511)
	at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:289)
	at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:316)
	at org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:301)
	at org.apache.kafka.log4jappender.KafkaLog4jAppender.getKafkaProducer(KafkaLog4jAppender.java:341)
	at org.apache.kafka.log4jappender.KafkaLog4jAppender.activateOptions(KafkaLog4jAppender.java:335)
	at org.apache.log4j.config.PropertySetter.activate(PropertySetter.java:307)
	at org.apache.log4j.config.PropertySetter.setProperties(PropertySetter.java:172)
	at org.apache.log4j.config.PropertySetter.setProperties(PropertySetter.java:104)
	at org.apache.log4j.PropertyConfigurator.parseAppender(PropertyConfigurator.java:842)
	at org.apache.log4j.PropertyConfigurator.parseCategory(PropertyConfigurator.java:768)
	at org.apache.log4j.PropertyConfigurator.parseCatsAndRenderers(PropertyConfigurator.java:672)
	at org.apache.log4j.PropertyConfigurator.doConfigure(PropertyConfigurator.java:516)
	at org.apache.log4j.PropertyConfigurator.doConfigure(PropertyConfigurator.java:580)
	at org.apache.log4j.helpers.OptionConverter.selectAndConfigure(OptionConverter.java:526)
	at org.apache.log4j.LogManager.<clinit>(LogManager.java:127)

I would expect any other project using the KafkaLog4JAppender to experience the same behavior.

I'm not sure why the KafkaLog4JAppender originally overrode the acks to 1, but it's certainly incompatible with the new idempotence default. An alternative would be to leave acks=1 and disable idempotence, but I figured fewer overrides make the appender easier to reason about.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@vvcephei , there's a getter API getRequiredNumAcks in KafkaLog4jAppender. After this change, the user would get default null from it if no override. I'm thinking we should set the default acks value to -1 or disable idempotenece here. WDYT?

ref: https://github.com/apache/kafka/blob/trunk/log4j-appender/src/main/java/org/apache/kafka/log4jappender/KafkaLog4jAppender.java#L99

@vvcephei
Copy link
Copy Markdown
Contributor Author

Thanks, @showuon ! I noticed that also, but there are also a whole bunch of other nullable object references with primitive-typed getters in that class, so I figured it was just some funky log4j thing.

I figured out that the setter is used by log4 to apply the configuration (for example, log4j.appender.kafka_appender.RequiredNumAcks=-1 calls setRequredNumAcks(-1)), but I don't know if the getters serve any purpose.

@showuon
Copy link
Copy Markdown
Member

showuon commented Feb 16, 2022

Yes, I don't know the getters serve any purpose, either. But I'm still worried that there's someone relying on it.
I'd like to hear what @hachikuji 's thought since he did the change to make requiredNumAcks to 1 in this PR: #5425 (comment)

@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Feb 17, 2022

Yes, I don't know the getters serve any purpose, either. But I'm still worried that there's someone relying on it.
I'd like to hear what @hachikuji 's thought since he did the change to make requiredNumAcks to 1 in this PR: #5425 (comment)

Unfair to mention a PR from 4 years ago 😉 . Regardless whether anyone actually uses this class, it does seem weird to have getters that we know are broken. I agree it seems better to initialize the defaults explicitly (and to use types consistently). Perhaps we may as well fix the other broken getters while we're at it. At a quick glance, it looks like maxBlockMs is the only other one?

@vvcephei
Copy link
Copy Markdown
Contributor Author

Closing in favor of https://issues.apache.org/jira/browse/KAFKA-13673 / #11788

@vvcephei vvcephei closed this Feb 17, 2022
@vvcephei vvcephei deleted the fix-log4j-appender branch February 17, 2022 16:11
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