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

[ISSUE #7074] Allow a BoundaryType to be specified when retrieving offset based on the timestamp #7082

Merged

Conversation

Koado
Copy link
Contributor

@Koado Koado commented Jul 25, 2023

Which Issue(s) This PR Fixes

[Feature] Allow a BoundaryType to be specified when retrieving offset based on the timestamp #7074

Brief Description

Add two interfaces to the DefaultMQAdminExt class for the reason that messages will miss if there are multiple messages with the same storeTime(endTime) when retrieving messages over a period of time.

  1. searchLowerBoundaryOffset
  2. searchUpperBoundaryOffset

How Did You Test This Change?

I wrote a test case to determine whether the maximum offset of the queue is equal to searchUpperBoundaryOffset(queue's maxTimestamp), and the minimum offset of the queue is equal to searchLowerBoundaryOffset(queue's minTimestamp). The codes are below:

String namesrvAddr = "127.0.0.1:9876";
        String topic = "test-topic";

        DefaultMQAdminExt mqAdminExt = new DefaultMQAdminExt();
        mqAdminExt.setInstanceName(UUID.randomUUID().toString());
        mqAdminExt.setNamesrvAddr(namesrvAddr);

        mqAdminExt.start();
        List<QueueTimeSpan> timeSpanList = mqAdminExt.queryConsumeTimeSpan(topic, null);
        if (timeSpanList != null && timeSpanList.size() > 0) {
            for (QueueTimeSpan timeSpan: timeSpanList) {
                MessageQueue mq = timeSpan.getMessageQueue();
                long maxOffset = mqAdminExt.maxOffset(mq);
                long minOffset = mqAdminExt.minOffset(mq);
                // if there is at least one message in queue, the maxOffset returns the queue's latest offset + 1
                assertThat((maxOffset == 0 ? 0 : maxOffset - 1) == mqAdminExt.searchUpperBoundaryOffset(mq, timeSpan.getMaxTimeStamp())).isTrue();
                assertThat(minOffset == mqAdminExt.searchLowerBoundaryOffset(mq, timeSpan.getMinTimeStamp())).isTrue();
            }
        }

@RongtongJin RongtongJin changed the title [Feature] Allow a BoundaryType to be specified when retrieving offset based on the timestamp [ISSUE #7074] Allow a BoundaryType to be specified when retrieving offset based on the timestamp Jul 26, 2023
@caigy
Copy link
Contributor

caigy commented Jul 27, 2023

Please fix failed tests.

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2023

Codecov Report

Merging #7082 (23eb3e8) into develop (90c5382) will increase coverage by 0.02%.
Report is 8 commits behind head on develop.
The diff coverage is 34.37%.

@@              Coverage Diff              @@
##             develop    #7082      +/-   ##
=============================================
+ Coverage      42.72%   42.75%   +0.02%     
- Complexity      9282     9296      +14     
=============================================
  Files           1138     1137       -1     
  Lines          81146    81267     +121     
  Branches       10619    10640      +21     
=============================================
+ Hits           34672    34746      +74     
- Misses         42137    42179      +42     
- Partials        4337     4342       +5     
Files Changed Coverage Δ
...a/org/apache/rocketmq/client/impl/MQAdminImpl.java 5.02% <0.00%> (-0.03%) ⬇️
...g/apache/rocketmq/client/impl/MQClientAPIImpl.java 23.52% <0.00%> (-0.04%) ⬇️
...ing/protocol/header/SearchOffsetRequestHeader.java 0.00% <0.00%> (ø)
...n/java/org/apache/rocketmq/store/ConsumeQueue.java 66.45% <ø> (+0.46%) ⬆️
...n/java/org/apache/rocketmq/store/MessageStore.java 0.00% <ø> (ø)
...che/rocketmq/tieredstore/TieredMessageFetcher.java 59.71% <ø> (ø)
...pache/rocketmq/tieredstore/TieredMessageStore.java 73.70% <ø> (ø)
...e/rocketmq/tieredstore/file/CompositeFlatFile.java 78.39% <ø> (ø)
.../rocketmq/tieredstore/file/TieredConsumeQueue.java 90.90% <ø> (ø)
...ache/rocketmq/tieredstore/file/TieredFlatFile.java 77.74% <ø> (ø)
... and 7 more

... and 26 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines 992 to 999
if (messageStore instanceof DefaultMessageStore) {
// get offset with specific boundary type
offset = ((DefaultMessageStore) messageStore).getOffsetInQueueByTime(requestHeader.getTopic(),
requestHeader.getQueueId(), requestHeader.getTimestamp(), requestHeader.getBoundaryType());
} else {
offset = messageStore.getOffsetInQueueByTime(requestHeader.getTopic(), requestHeader.getQueueId(),
requestHeader.getTimestamp());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I found that getOffsetInQueueByTime also exists in TieredMessageStore. Would it be better to abstract an interface instead of checking the type every time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also wanted to add an interface directly at the beginning. But later I found that the BoundaryType used in TieredMessageStore belongs to package org.apache.rocketmq.tieredstore.common and the BoundaryType used in DefaultMessageStore belongs to package org.apache.rocketmq.common. So if I wanna to abstract an interface, the argument of the interface might be boundaryTypeName(String), which means that the enum class is converted to a string, or the string is converted to the enum class, each time the argument is passed and the argument is fetched.
Or maybe I should unify the two enum classes?What do you think I should do in this situation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to unify the two enumeration classes.

@RongtongJin RongtongJin merged commit 1fe5d62 into apache:develop Aug 3, 2023
10 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.

4 participants