MINOR: Update Consumer and Producer JavaDocs for committing offsets by mjsax · Pull Request #18336 · apache/kafka

mjsax · 2024-12-28T00:38:47Z

The consumer/producer JavaDocs still contain instruction for naively computing the offset to be committed.
This PR updates the JavaDocs with regard to the improvements of KIP-1094.

The consumer/producer JavaDocs still contain instruction for naively computing the offset to be committed. This PR updates the JavaDocs with regard to the improvements of KIP-1094.

AndrewJSchofield

Thanks for the PR. A few small comments, but looks like a good improvement.

AndrewJSchofield · 2024-12-28T08:19:19Z

- * which is at position 5 has consumed records with offsets 0 through 4 and will next receive the record with offset 5. There
- * are actually two notions of position relevant to the user of the consumer:
+ * which is at position 5 has consumed records with offsets 0 through 4 and will next receive the record with offset 5.
+ * Note that offsets are not guaranteed to be consecutive (eg., for compacted topic, or&mdash;independent of "read_committed"


I suggest the following the parentheses "(such as compacted topic or when records have been produced using transactions)".

AndrewJSchofield · 2024-12-28T08:19:42Z

+ * which is at position 5 has consumed records with offsets 0 through 4 and will next receive the record with offset 5.
+ * Note that offsets are not guaranteed to be consecutive (eg., for compacted topic, or&mdash;independent of "read_committed"
+ * mode&mdash; transactional topics). For example, if the consumer did read a record with offset 4, but 5 is not an offset
+ * with a record, it's position might advance to 6 (or higher) directly. Similarly, if the consumer's position is 5,


nit: its not it's

AndrewJSchofield · 2024-12-28T08:33:01Z

     * should not be used. The committed offset should be the next message your application will consume,
-     * i.e. lastProcessedMessageOffset + 1. If automatic group management with {@link #subscribe(Collection)} is used,
+     * i.e. {@code nextRecordToBeProcessed.offset()} (or {@link ConsumerRecords#nextOffsets()}).
+     * You should also add the {@link ConsumerRecord#leaderEpoch()} (or {@code nextOffsets().get(...).leaderEpoch()})


Maybe "You should also add the leader epoch as commit metadata, which can be obtained from {@link ConsumerRecord#leaderEpoch()} or {@link ConsumerRecords#nextOffsets}." I didn't find the nextOffsets().get(...).leaderEpoch() that easy to follow and the hyperlink to the ConsumerRecords seems nicer to me.

AndrewJSchofield · 2024-12-28T08:33:34Z

     * should not be used. The committed offset should be the next message your application will consume,
-     * i.e. lastProcessedMessageOffset + 1. If automatic group management with {@link #subscribe(Collection)} is used,
+     * i.e. {@code nextRecordToBeProcessed.offset()} (or {@link ConsumerRecords#nextOffsets()}).
+     * You should also add the {@link ConsumerRecord#leaderEpoch()} (or {@code nextOffsets().get(...).leaderEpoch()})


Same point as above about nextOffsets().get(...).leaderEpoch().

AndrewJSchofield · 2024-12-28T08:33:44Z

     * should not be used. The committed offset should be the next message your application will consume,
-     * i.e. lastProcessedMessageOffset + 1. If automatic group management with {@link #subscribe(Collection)} is used,
+     * i.e. {@code nextRecordToBeProcessed.offset()} (or {@link ConsumerRecords#nextOffsets()}).
+     * You should also add the {@link ConsumerRecord#leaderEpoch()} (or {@code nextOffsets().get(...).leaderEpoch()})


AndrewJSchofield · 2024-12-28T08:34:08Z

-     * be the next message your application will consume, i.e. lastProcessedMessageOffset + 1.
+     * be the next message your application will consume, i.e. {@code nextRecordToBeProcessed.offset()}
+     * (or {@link ConsumerRecords#nextOffsets()}). You should also add the {@link ConsumerRecord#leaderEpoch()}
+     * (or {@code nextOffsets().get(...).leaderEpoch()}) as commit metadata.


chia7712

@mjsax thanks for this patch. one small comment is left. PTAL

chia7712 · 2025-01-01T13:28:35Z

 *                 }
- *                 long lastOffset = partitionRecords.get(partitionRecords.size() - 1).offset();
- *                 consumer.commitSync(Collections.singletonMap(partition, new OffsetAndMetadata(lastOffset + 1)));
+ *                 consumer.commitSync(Collections.singletonMap(partition, partitionRecords.nextOffsets().get(partition));


typo: partitionRecords -> records

and missing parenthesis, I guess we should end with :

Suggested change

* consumer.commitSync(Collections.singletonMap(partition, partitionRecords.nextOffsets().get(partition));

* consumer.commitSync(Collections.singletonMap(partition, records.nextOffsets().get(partition)));

partitionRecords is actually correct; nextRecords() was added to ConsumerRecord[s] (not ConsumerRecord) -- that's also why we need to call get(partition) -- ConsumerRecord is already data for a single partition, and get(partition) would not make sense.

@mjsax Could you please take a look at following attachment? the type of partitionRecords is List<ConsumerRecord<String, String>> rather than ConsumerRecords

btw, it miss a ) also.

Thanks for sanity checking... I did mix up the variable names.

lianetm

Thanks for the nice update! Just couple of comments (and agree with the previous suggestions)

lianetm · 2025-01-02T15:26:20Z

+ * mode&mdash; transactional topics). For example, if the consumer did read a record with offset 4, but 5 is not an offset
+ * with a record, it's position might advance to 6 (or higher) directly. Similarly, if the consumer's position is 5,
+ * but there is no record with offset 5, the consumer will return the record with the next higher offset.
+ * There are actually two notions of position relevant to the user of the consumer:


Related to this but on ln 84 right below (sorry couldn't add comment there):

The {@link #commitSync() committed position} is the last offset that has been stored securely

shouldn't that refer to committed(..) instead?

The {@link #committed(Set) committed position} is the last offset that has been stored securely

Not sure... I did not modify this part... There are actually two notions of position relevant to the user of the consumer: did not change.

But I think it ok as is? Guess it works both ways... It's just a difference between "write path" vs "read path", right?

Let me know what you think. If you think using #commited(Set) is better, happy to update it, but it's orthogonal to what I want to do in this PR and would be some side improvement.

lianetm · 2025-01-02T15:28:53Z

 *                 }
- *                 long lastOffset = partitionRecords.get(partitionRecords.size() - 1).offset();
- *                 consumer.commitSync(Collections.singletonMap(partition, new OffsetAndMetadata(lastOffset + 1)));
+ *                 consumer.commitSync(Collections.singletonMap(partition, partitionRecords.nextOffsets().get(partition));


and missing parenthesis, I guess we should end with :

Suggested change

* consumer.commitSync(Collections.singletonMap(partition, partitionRecords.nextOffsets().get(partition));

* consumer.commitSync(Collections.singletonMap(partition, records.nextOffsets().get(partition)));

mjsax · 2025-01-03T18:37:52Z

Thanks for all the input. Pushed an updated.

AndrewJSchofield

Just one final comment.

AndrewJSchofield · 2025-01-06T16:42:54Z

- * Thus, when calling {@link #commitSync(Map) commitSync(offsets)} you should add one to the offset of the last message processed.
+ * Thus, when calling {@link #commitSync(Map) commitSync(offsets)} you should use {@code nextRecordToBeProcessed.offset()}
+ * or if {@link ConsumerRecords} is exhausted already {@link ConsumerRecords#nextOffsets()} instead.
+ * You should also pass in the {@link ConsumerRecord#leaderEpoch()} (or {@code nextOffsets().get(...).leaderEpoch()})


In the other similar places, you use the You should also add the leader epoch as commit metadata, which can be obtained from {@link ConsumerRecord#leaderEpoch()} or {@link ConsumerRecords#nextOffsets()}.. I think consistency would be good here.

AndrewJSchofield

Thanks for the PR. Looks good to me.

…18336) The consumer/producer JavaDocs still contain instruction for naively computing the offset to be committed. This PR updates the JavaDocs with regard to the improvements of KIP-1094. Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>, Lianet Magrans <lmagrans@confluent.io>

mjsax · 2025-01-06T21:43:21Z

Merged to trunk and cherry-picked to 4.0 branch.

…og-compaction-write-record-v2 * apache-github/trunk: (34 commits) MINOR: Bump year to 2025 in NOTICE file (apache#18427) KAFKA-18411 Remove ZkProducerIdManager (apache#18413) KAFKA-18408 tweak the 'tag' field for BrokerHeartbeatRequest.json, BrokerRegistrationChangeRecord.json and RegisterBrokerRecord.json (apache#18421) KAFKA-18414 Remove KRaftRegistrationResult (apache#18401) KAFKA-17921 Support SASL_PLAINTEXT protocol with java.security.auth.login.config (apache#17671) KAFKA-18384 Remove ZkAlterPartitionManager (apache#18364) KAFKA-10790: Add deadlock detection to producer#flush (apache#17946) KAFKA-18412: Remove EmbeddedZookeeper (apache#18399) MINOR : Improve Exception log in NotEnoughReplicasException(apache#12394) MINOR: Improve PlaintextAdminIntegrationTest#testConsumerGroups (apache#18409) MINOR: Remove unused local variable (apache#18410) MINOR: Remove RaftManager.maybeDeleteMetadataLogDir and AutoTopicCreationManagerTest.scala (apache#17365) KAFKA-18368 Remove TestUtils#MockZkConnect and remove zkConnect from TestUtils#createBrokerConfig (apache#18352) MINOR: Update Consumer group timeout default to 30 sec (apache#16406) MINOR: Fix typo in CommitRequestManager (apache#18407) MINOR: cleanup JavaDocs for deprecation warnings (apache#18402) KAFKA-18303; Update ShareCoordinator to use new record format (apache#18396) MINOR: Update Consumer and Producer JavaDocs for committing offsets (apache#18336) KAFKA-16446: Improve controller event duration logging (apache#15622) KAFKA-18388 test-kraft-server-start.sh should use log4j2.yaml (apache#18370) ...

…pache#18336) The consumer/producer JavaDocs still contain instruction for naively computing the offset to be committed. This PR updates the JavaDocs with regard to the improvements of KIP-1094. Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>, Lianet Magrans <lmagrans@confluent.io>

MINOR: Update Consumer and Producer JavaDocs for committing offsets

df949d3

The consumer/producer JavaDocs still contain instruction for naively computing the offset to be committed. This PR updates the JavaDocs with regard to the improvements of KIP-1094.

mjsax added docs producer consumer labels Dec 28, 2024

github-actions Bot added clients small Small PRs labels Dec 28, 2024

AndrewJSchofield requested changes Dec 28, 2024

View reviewed changes

chia7712 reviewed Jan 1, 2025

View reviewed changes

lianetm reviewed Jan 2, 2025

View reviewed changes

mjsax added 2 commits January 2, 2025 13:56

review comments

56716c0

review comment

8bae23b

AndrewJSchofield requested changes Jan 6, 2025

View reviewed changes

github comment

eae5541

AndrewJSchofield approved these changes Jan 6, 2025

View reviewed changes

mjsax merged commit 3918f37 into apache:trunk Jan 6, 2025

mjsax deleted the update-consumer-producer-javadocs branch January 6, 2025 21:39

	* consumer.commitSync(Collections.singletonMap(partition, partitionRecords.nextOffsets().get(partition));
	* consumer.commitSync(Collections.singletonMap(partition, records.nextOffsets().get(partition)));

Conversation

mjsax commented Dec 28, 2024

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jan 3, 2025

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jan 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mjsax Jan 2, 2025 •

edited

Loading