Skip to content

Conversation

@PratRanj07
Copy link
Contributor

Implemented fetchTopicOffsetsByTimestamp for kafkajs and wrote tests and example for it

@PratRanj07 PratRanj07 requested review from a team as code owners December 12, 2024 07:48
@confluent-cla-assistant
Copy link

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

Thanks!
Some changes suggested.
Also update the changelog and the readme

@PratRanj07 PratRanj07 requested a review from milindl December 16, 2024 12:38
Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

As a part of this PR, add

@memberof RdKafka

Within admin.js, for the comment for the function OffsetSpec

CHANGELOG.md Outdated
2. Add support for Node v23 pre-built binaries (#158).
3. Add KafkaJS-compatible errors to promisified Admin API (createTopics, deleteGroups, deleteTopicRecords) (#159).
4. Include error types within Type definitions for promisified API (#210).
5. Add support for an Admin API to fetch topic offsets by timestamp (#206).
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to changelog for 1.1.0 as discussed

},
]);

const offsetsAtSpecificTimestamp2 = await admin.fetchTopicOffsetsByTimestamp(topicName, now + 150);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const offsetsAtSpecificTimestamp2 = await admin.fetchTopicOffsetsByTimestamp(topicName, now + 150);
const offsetsAtSpecificTimestamp2 = await admin.fetchTopicOffsetsByTimestamp(topicName, now + 250);

(tests the 'equal' case)

Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

LGTM!

@PratRanj07 PratRanj07 merged commit 65451d9 into master Dec 17, 2024
2 checks passed
@PratRanj07 PratRanj07 deleted the feature/fetchTopicOffsetsByTimestamp branch December 17, 2024 05:58
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