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

MINOR: Specify 2.1 as the minimum broker version for clients #19250

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

ijuma
Copy link
Member

@ijuma ijuma commented Mar 19, 2025

Also fix a few other client javadoc issues.

@ijuma ijuma force-pushed the consumer-javadoc-minimum-broker-version branch from a02e97c to 4c1055e Compare March 20, 2025 04:30
@github-actions github-actions bot added producer and removed small Small PRs labels Mar 20, 2025
* certain features. For example, 0.10.0 brokers do not support offsetsForTimes, because this feature was added
* in version 0.10.1. You will receive an {@link org.apache.kafka.common.errors.UnsupportedVersionException}
* This client can communicate with brokers that are version 2.1 or newer. Older or newer brokers may not support
* certain features. For example, 3.9 brokers do not support {@code subscribe(SubscriptionPattern)}, because this feature was added
Copy link
Member Author

Choose a reason for hiding this comment

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

@lianetm @dajac Is this accurate?

Copy link
Member

Choose a reason for hiding this comment

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

yes, it's accurate. That feature was the last one added as part of KIP-848 (in 4.0).

* By default a buffer is available to send immediately even if there is additional unused space in the buffer. However if you
* want to reduce the number of requests you can set <code>linger.ms</code> to something greater than 0. This will
* instruct the producer to wait up to that number of milliseconds before sending a request in hope that more records will
* By default, a buffer is available to send after a short delay even if there is additional unused space in the buffer.
Copy link
Member Author

Choose a reason for hiding this comment

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

The linger.ms default changed in 4.0.

* then it is recommended to shut down the producer and check the contents of the last produced message to ensure that
* it is not duplicated. Finally, the producer can only guarantee idempotence for messages sent within a single session.
* To ensure idempotence, it is imperative to avoid application level re-sends since these cannot be de-duplicated.
* To achieve this, it is recommended to set {@code delivery.timeout.ms} such that retries are handled for the desired
Copy link
Member Author

Choose a reason for hiding this comment

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

We updated a different part of the javadoc to emphasize delivery.timeout.ms, but forgot to update this part.

* <code>UnsupportedVersionException</code> when invoking an API that is not available in the running broker version.
* This client can communicate with brokers that are version 2.1 or newer. Older brokers may not support
* certain client features. For instance, {@code sendOffsetsToTransaction} with all consumer group metadata needs broker
* versions 2.5 or later. You will receive an <code>UnsupportedVersionException</code> when invoking an API that is not
Copy link
Member Author

Choose a reason for hiding this comment

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

@jolshan Is this correct?

Copy link
Member

@jolshan jolshan Mar 20, 2025

Choose a reason for hiding this comment

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

I was a little confused by this at first, so I did some digging.
This message is due to the change in the signature of sendOffsetsToTransaction requiring ConsumerGroupMetadata groupMetadata rather than String consumerGroupId (KIP introducing the concept here, and old method signature deprecated in this commit)

But yes, this is correct. 👍

Copy link
Member

Choose a reason for hiding this comment

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

We could just remove the "with all consumer group metadata" comment, since this is the only remaining version of the method. (I guess you can't add offsets to transactions if you are running 2.4 or older)

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, not sure if this implication was fully understood. I'll check what other updates are required due to this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't think it was when it was deprecated.

@@ -636,8 +632,6 @@ private TransactionManager configureTransactionState(ProducerConfig config,
* initialized, this method should no longer be used.
*
* @throws IllegalStateException if no {@code transactional.id} has been configured
* @throws org.apache.kafka.common.errors.UnsupportedVersionException fatal error indicating the broker
Copy link
Member Author

Choose a reason for hiding this comment

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

@jolshan Thoughts on this and similar changes? Two other options:

  1. Keep UnsupportedVersionException with a generic message (instead of the specific "does not support transactions").
  2. Keep UnsupportedVersionException with a different specific message.

Perhaps alternative 1 is the right approach if we expect to evolve things such that passing certain parameters may result in UnsupportedVersionException. Similar to sendOffsetsToTransaction with all consumer group metadata.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think 1 makes sense to leave a possibility for a more specific error than KafkaException.
When we complete KIP-1050 we will have error classes so maybe this is less important though.

@ijuma
Copy link
Member Author

ijuma commented Mar 20, 2025

This needs to be cherry-picked to the 4.0 branch.

@ijuma ijuma marked this pull request as ready for review March 20, 2025 04:36
* This client can communicate with brokers that are version 0.10.0 or newer. Older or newer brokers may not support
* certain client features. For instance, the transactional APIs need broker versions 0.11.0 or later. You will receive an
* <code>UnsupportedVersionException</code> when invoking an API that is not available in the running broker version.
* This client can communicate with brokers that are version 2.1 or newer. Older brokers may not support
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the "or newer" from here and the consumer documentation because I couldn't think of any example of where that is true. Am I missing something @dajac @junrao ?

Copy link
Contributor

Choose a reason for hiding this comment

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

For producers, transaction.version needs to be set on the newer server to enable txn V2 support.

For consumers, group.version needs to be set to enable the new consumer rebalance protocol.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. But I think the message as written before made it sound like a newer broker version may result in some consumer/producer features not working. That seems unlikely to me. The more likely thing is that a newer broker would mean a certain consumer version is no longer supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants