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.

Sign up for GitHub

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

Jump to bottom

[Issue 10896] add get message id by timestamp #11139

Merged
merged 4 commits into from
Jul 5, 2021
Merged

[Issue 10896] add get message id by timestamp #11139

merged 4 commits into from
Jul 5, 2021

Conversation

Jason918
Copy link
Contributor

@Jason918 Jason918 commented Jun 29, 2021 •
edited
Loading

Fixes #10896

Motivation

Add getMessageIdByTimestamp in pulsar admin apis

Modifications

The core searching implementation reuses org.apache.bookkeeper.mledger.ManagedLedger#asyncFindPosition.
Add client tool for cmd pulsar-admin topics get-message-id

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Extended integration test in org.apache.pulsar.broker.admin.PersistentTopicsTest#testGetMessageIdByTimestamp

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (yes)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (yes)
  • The admin cli options: (yes)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (docs and JavaDocs)

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

@codelipenghui Hi, can you please take a look?

@codelipenghui codelipenghui added this to the 2.9.0 milestone Jun 29, 2021
codelipenghui
codelipenghui reviewed Jun 29, 2021
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Good job! Just left some minor comments

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java Outdated Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java Show resolved Hide resolved
Anonymitaet
Anonymitaet requested changes Jun 29, 2021
View reviewed changes
Copy link
Member

@Anonymitaet Anonymitaet left a comment

Choose a reason for hiding this comment

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

Many thanks for adding docs! I've left some comments, PTAL.

pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java Outdated Show resolved Hide resolved
site2/docs/admin-api-topics.md Outdated Show resolved Hide resolved
site2/docs/admin-api-topics.md Outdated Show resolved Hide resolved
@Anonymitaet Anonymitaet added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jun 29, 2021
eolivelli
eolivelli requested changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Overall LGTM

how does this feature play with batched messages?
It looks to me that the MessageIdImpl that we are returning is not compatible with batched messages.

Can you please add a unit test?

@Jason918 @Anonymitaet
Update site2/docs/admin-api-topics.md
585306e 
Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
@Jason918
Copy link
Contributor Author

Jason918 commented Jul 2, 2021

@eolivelli Hi, Thank you for the reminding, I have added testGetBatchMessageIdByTimestamp for batch message case.
Batch messages are stored in the same entry, which should be considered stored at the same time. So only message id containing ledgerId and entryId is returned.

@eolivelli
Copy link
Contributor

Batch messages are stored in the same entry, which should be considered stored at the same time
Very interesting point !

Thanks for the new test

eolivelli
eolivelli approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@codelipenghui codelipenghui merged commit a7e40ea into apache:master Jul 5, 2021
@freeznet freeznet mentioned this pull request Jul 5, 2021
Merged
1 task
tuteng pushed a commit that referenced this pull request Jul 6, 2021
@freeznet
fix deserializeBrokerEntryMetaDataFirst not found (#11220)
ad01fc9 
### Motivation

#11014 introduced `getEntryTimestamp` and removed `deserializeBrokerEntryMetaDataFirst`, but #11139 is still using `deserializeBrokerEntryMetaDataFirst`, and it breaks master branch.

### Modifications

use `entryTimestamp` for expiry checking in `internalGetMessageIdByTimestamp`

### Verifying this change

- [x] Make sure that the change passes the CI checks.
@Jason918 Jason918 deleted the issue-10896 branch July 26, 2021 02:55
Jason918 added a commit to Jason918/pulsar that referenced this pull request Jan 25, 2022
@Jason918
[Issue 10896] add get message id by timestamp (apache#11139)
8bbb0a2 
Fixes apache#10896

Add getMessageIdByTimestamp in pulsar admin apis

The core searching implementation reuses `org.apache.bookkeeper.mledger.ManagedLedger#asyncFindPosition`.
Add client tool for cmd `pulsar-admin topics get-message-id`
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@Jason918 @ciaocloud
[Issue 10896] add get message id by timestamp (apache#11139)
8c284e0 
Fixes apache#10896

### Motivation

Add getMessageIdByTimestamp in pulsar admin apis

### Modifications

The core searching implementation reuses `org.apache.bookkeeper.mledger.ManagedLedger#asyncFindPosition`.
Add client tool for cmd `pulsar-admin topics get-message-id`
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@freeznet @ciaocloud
fix deserializeBrokerEntryMetaDataFirst not found (apache#11220)
e147b34 
### Motivation

apache#11014 introduced `getEntryTimestamp` and removed `deserializeBrokerEntryMetaDataFirst`, but apache#11139 is still using `deserializeBrokerEntryMetaDataFirst`, and it breaks master branch.

### Modifications

use `entryTimestamp` for expiry checking in `internalGetMessageIdByTimestamp`

### Verifying this change

- [x] Make sure that the change passes the CI checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@sijie sijie sijie approved these changes

@eolivelli eolivelli eolivelli approved these changes

@Anonymitaet Anonymitaet Anonymitaet approved these changes

@massakam massakam Awaiting requested review from massakam

@315157973 315157973 Awaiting requested review from 315157973

@congbobo184 congbobo184 Awaiting requested review from congbobo184

@gaoran10 gaoran10 Awaiting requested review from gaoran10

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@zymap zymap Awaiting requested review from zymap

Assignees

@Jason918 Jason918

Labels
area/admin doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.

Get message id based on timestamp
5 participants
@Jason918 @eolivelli @sijie @codelipenghui @Anonymitaet