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

feat: add timestamp-query feature to allow timestamp offset query #248

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

fMeow
Copy link
Contributor

@fMeow fMeow commented Oct 21, 2024

This commit allow timestamp query for partition offset. As stated in the doc comment of OffsetAt, timestamp-based query is inconsistent, but I think it better to let the users make their own choice instead of total removal of this sometimes useful feature. I add a feature gate to disable timestamp-based query for offset by default, only those actively looking for timestamp-based query would enable this feature.

  • I've read the contributing section of the project CONTRIBUTING.md.
  • Signed CLA (if not already signed).

Copy link
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

It's definitely a feature that we should support, but it needs a bit more thorough API design (see code-attached comment) and a test.

Cargo.toml Outdated Show resolved Hide resolved
src/client/partition.rs Outdated Show resolved Hide resolved
@fMeow
Copy link
Contributor Author

fMeow commented Oct 21, 2024

I have change the code for a better API with DateTime<Utc>. Would add tests later.

@fMeow
Copy link
Contributor Author

fMeow commented Nov 14, 2024

Sorry for the long wait. I have added test to query timestamp offset. @crepererum

tests/consumer.rs Outdated Show resolved Hide resolved
tests/consumer.rs Outdated Show resolved Hide resolved
tests/consumer.rs Outdated Show resolved Hide resolved
tests/consumer.rs Outdated Show resolved Hide resolved
src/client/partition.rs Show resolved Hide resolved
src/client/consumer.rs Outdated Show resolved Hide resolved
@fMeow
Copy link
Contributor Author

fMeow commented Nov 22, 2024

Pls have a look. @crepererum

Copy link
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the nitpicks and providing a very clean PR. Good job!

@crepererum
Copy link
Collaborator

Ah, last minute issue: could you squash all commits into a single one and name it:

feat: add timestamp-query feature to allow timestamp offset query

@fMeow
Copy link
Contributor Author

fMeow commented Nov 22, 2024

done

@crepererum
Copy link
Collaborator

Sorry for the CI failure, this will get fixed by #251 (seems to be bad timing).

@fMeow
Copy link
Contributor Author

fMeow commented Nov 23, 2024

@crepererum seems good now.

@crepererum crepererum merged commit 75c5a2a into influxdata:main Nov 25, 2024
11 checks passed
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.

2 participants