KAFKA-12574: KIP-732, Deprecate eos-alpha and replace eos-beta with eos-v2 #10573
Conversation
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/producer/Producer.java
Outdated
Show resolved
Hide resolved
streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java
Outdated
Show resolved
Hide resolved
|
@mjsax thanks for the review, addressed your comments please give it another pass |
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
Outdated
Show resolved
Hide resolved
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java
Outdated
Show resolved
Hide resolved
...ms/src/test/java/org/apache/kafka/streams/integration/ResetPartitionTimeIntegrationTest.java
Show resolved
Hide resolved
streams/src/main/java/org/apache/kafka/streams/processor/internals/ActiveTaskCreator.java
Show resolved
Hide resolved
streams/src/test/java/org/apache/kafka/streams/processor/internals/RecordCollectorTest.java
Show resolved
Hide resolved
streams/src/test/java/org/apache/kafka/streams/processor/internals/RecordCollectorTest.java
Show resolved
Hide resolved
ableegoldman
force-pushed
the
12574-deprecate-eos-configs-and-sendOffsetsToTransaction
branch
from
9a2013f
to
49a33f6
Compare
…precate-eos-configs-and-sendOffsetsToTransaction
|
Tons of flaky test failures, all unrelated. Mostly Connect and Raft, a few Streams tests that are known to be flaky which I left some thoughts on the ticket for. Going to merge |
|
Merged to trunk |
| @@ -525,6 +525,11 @@ private TransactionManager configureTransactionState(ProducerConfig config, | |||
| final int transactionTimeoutMs = config.getInt(ProducerConfig.TRANSACTION_TIMEOUT_CONFIG); | |||
| final long retryBackoffMs = config.getLong(ProducerConfig.RETRY_BACKOFF_MS_CONFIG); | |||
| final boolean autoDowngradeTxnCommit = config.getBoolean(ProducerConfig.AUTO_DOWNGRADE_TXN_COMMIT); | |||
| // Only log a warning if being used outside of Streams, which we know includes "StreamThread-" in the client id | |||
| if (autoDowngradeTxnCommit && !clientId.contains("StreamThread-")) { | |||
There was a problem hiding this comment.
We should not have Streams specific code in the producer.
There was a problem hiding this comment.
In general I agree, however given that we intend to remove it in 4.0 (that should not be too long out), it seems acceptable? If you feel strong about it, any proposal how to avoid it?
There was a problem hiding this comment.
4.0 is probably 18 months away, that's a reasonably long time. Why do we need to log a warning only if it's not Streams?
There was a problem hiding this comment.
@ijuma I came across this after the question was raised around the autodowngrade logic, apparently (according to the config's javadocs) it's an "internal" config that's only used for Streams. The config itself is package-private.
Given that, I thought we may want to log a warning to any plain client users that saw this config and didn't notice that it was internal, and thus tried to use it. But I'm happy to do a followup PR to remove this. Alternatively, we can just take this config out -- I actually don't see any reason why it should be necessary, AFAICT it's just a slight convenience config that saves Streams from the ~5 lines of code it would take to do this downgrade itself (basically it just erases the extra consumer group metadata that isn't understood by older brokers). Not sure if this was vestigial from an older iteration of KIP-447, as it seems rather unnecessary..
There was a problem hiding this comment.
@ableegoldman Your suggestion to remove the config altogether seems best to me. We don't have a grace period for internal configs, that's why they're internal. :)
| @@ -179,10 +179,18 @@ public void beginTransaction() throws ProducerFencedException { | |||
| this.sentOffsets = false; | |||
| } | |||
|
|
|||
| @SuppressWarnings("deprecation") | |||
There was a problem hiding this comment.
Shouldn't this have a deprecated annotation instead of the warning suppression?
There was a problem hiding this comment.
Would be better I guess. But doesn't it inherit the @Deprecated annotation automatically?
There was a problem hiding this comment.
I guess it doesn't need to have either, I can remove in a followup PR
There was a problem hiding this comment.
Actually no, we do get a warning if we don't have either annotation. I'll change it to @Deprecated then
…#10675) Minor followup to #10573. Removes this internal Producer config which was only ever used to avoid a very minor amount of work to downgrade the consumer group metadata in the txn commit request in Kafka Streams Reviewers: Ismael Juma <[email protected]>, Matthias J. Sax <[email protected]>, Guozhang Wang <[email protected]>
Deprecates and logs a warning upon usage of the following:
The deprecated eos configs are to be replaced by the new StreamsConfig.EXACTLY_ONCE_V2 config. Additionally, this PR replaces usages of the term "eos-beta" throughout the code with the term "eos-v2"