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

[RIP 30] Support Compaction topic #4118

Merged
merged 13 commits into from
Jul 8, 2022

Conversation

ltamber
Copy link
Contributor

@ltamber ltamber commented Apr 5, 2022

refer to #3799
the implement was base on 5.0.0-beta

@coveralls
Copy link

coveralls commented Apr 5, 2022

Coverage Status

Coverage decreased (-0.2%) to 47.024% when pulling f4ce8b6 on ltamber:5.0.0-beta-ctopic into 52482d4 on apache:5.0.0-beta.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2022

Codecov Report

Merging #4118 (9f9f082) into 5.0.0-beta (96e9e48) will decrease coverage by 0.11%.
The diff coverage is 43.33%.

@@               Coverage Diff                @@
##             5.0.0-beta    #4118      +/-   ##
================================================
- Coverage         43.14%   43.02%   -0.12%     
- Complexity         6058     6153      +95     
================================================
  Files               801      810       +9     
  Lines             57086    57965     +879     
  Branches           7805     7923     +118     
================================================
+ Hits              24630    24941     +311     
- Misses            29230    29765     +535     
- Partials           3226     3259      +33     
Impacted Files Coverage Δ
...g/apache/rocketmq/acl/common/AclClientRPCHook.java 82.85% <ø> (+2.30%) ⬆️
...a/org/apache/rocketmq/broker/BrokerController.java 49.17% <ø> (+0.05%) ⬆️
...g/apache/rocketmq/client/impl/MQClientAPIImpl.java 24.54% <0.00%> (+0.06%) ⬆️
...apache/rocketmq/common/attribute/DeletePolicy.java 0.00% <0.00%> (ø)
...pache/rocketmq/common/utils/DeletePolicyUtils.java 0.00% <0.00%> (ø)
...rocketmq/remoting/netty/NettyRemotingAbstract.java 49.82% <ø> (+1.04%) ⬆️
...e/rocketmq/remoting/netty/NettyRemotingClient.java 42.76% <ø> (-0.16%) ⬇️
...org/apache/rocketmq/store/AppendMessageResult.java 63.63% <0.00%> (-3.04%) ⬇️
...ava/org/apache/rocketmq/store/MappedFileQueue.java 52.91% <0.00%> (-1.95%) ⬇️
...n/java/org/apache/rocketmq/store/MessageStore.java 0.00% <ø> (ø)
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13ffac8...9f9f082. Read the comment docs.

@ltamber ltamber changed the title [ISSUE #3799] support compaction topic [RIP 30] Support Compaction topic Apr 7, 2022
@duhenglucky duhenglucky added this to the 5.0.0-beta milestone Apr 7, 2022
Copy link
Contributor

@RongtongJin RongtongJin left a comment

Choose a reason for hiding this comment

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

It would be better to add an integration test

CompactionLog log = new CompactionLog(defaultMessageStore, topic, queueId,
offsetMapSize, positionMgr, compactionLogPath, compactionCqPath);
compactionLogTable.put(topic + "_" + queueId, log);
compactionSchedule.scheduleWithFixedDelay(log::doCompaction, compactionInterval, compactionInterval, TimeUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Different compactionLog files are distinguished by topic and queueId, and do compaction in every compactionLog. Does this mean messages with the same keys must send to the same queue?

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.

MappedFile lastMappedFile = this.mappedFileQueue.getLastMappedFile();
if (lastMappedFile == null) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I need to add synchronized or lock to ensure thread safety when restoring a ByteBuffer from native?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the method recover invoked by only one thread, I think there is no need to add synchronized or lock

@ltamber ltamber changed the base branch from 5.0.0-beta to 5.0.0-beta-compact July 8, 2022 06:49
@duhenglucky duhenglucky merged commit ca94afc into apache:5.0.0-beta-compact Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants