-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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 #7064] [RIP-66] Support KV(RocksDB) Storage #7065
Conversation
@@ -27,7 +29,7 @@ | |||
|
|||
import static org.apache.rocketmq.common.TopicAttributes.TOPIC_MESSAGE_TYPE_ATTRIBUTE; | |||
|
|||
public class TopicConfig { | |||
public class TopicConfig implements Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不建议这么改
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已删除
@@ -104,5 +104,10 @@ | |||
<groupId>io.github.aliyunmq</groupId> | |||
<artifactId>rocketmq-logback-classic</artifactId> | |||
</dependency> | |||
<!-- rocksdb --> | |||
<dependency> | |||
<groupId>io.github.aliyunmq</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rely on a commercial version? How to ensure the stability of this version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still open source, but we need a new compactionFilter for ConsumeQueue. If you're interested, see how flink uses rocksdb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fujian-zfj Your purpose of building custom rocksdb is to define a compaction filter. What a custom compaction filter expects is native function. Will this way be better alternative:
- Use official rocksdb-java jar;
- Build a shared library(dynamically linked with rocksdb shared library) for expected custom compaction filters;
- When configuring RocksDB, pass the native function to it, because it does not matter which shared library the compaction filter stays in;
As a matter of fact, this is how the native programming languages use rocksdb in case of dynamic link.
<groupId>io.github.aliyunmq</groupId> | ||
<artifactId>rocketmq-rocksdb</artifactId> | ||
<version>${rocksdb.version}</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this feature compatible with other platforms (Windows and macOS)? If not, should we set it as optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
support Windows, MacOS and Linux
Codecov Report
@@ Coverage Diff @@
## develop #7065 +/- ##
=============================================
- Coverage 42.72% 42.64% -0.08%
- Complexity 9282 9480 +198
=============================================
Files 1138 1157 +19
Lines 81146 83192 +2046
Branches 10619 10850 +231
=============================================
+ Hits 34672 35481 +809
- Misses 42137 43297 +1160
- Partials 4337 4414 +77
... and 26 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
this.flushOptions.setAllowWriteStall(false); | ||
|
||
this.writeBatch = new WriteBatch(); | ||
this.bufferDRList = new ArrayList(BATCH_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raw use of parameterized class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
It's a huge pull request, containing massive changes to core parts of this project with more than 6000 lines of change. Thus, it's challenging to have a quality review. Normally, developers would break it into a series of small and verifiable ones to keep community participation. |
@@ -746,7 +760,12 @@ public boolean initializeMetadata() { | |||
public boolean initializeMessageStore() { | |||
boolean result = true; | |||
try { | |||
DefaultMessageStore defaultMessageStore = new DefaultMessageStore(this.messageStoreConfig, this.brokerStatsManager, this.messageArrivingListener, this.brokerConfig, topicConfigManager.getTopicConfigTable()); | |||
DefaultMessageStore defaultMessageStore; | |||
if (isEnableRocksDBStore()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will SPI be a better choice than if...else
?
@@ -225,7 +225,7 @@ public void shutdown() { | |||
|
|||
public synchronized void changeBrokerRole(final Long newMasterBrokerId, final String newMasterAddress, | |||
final Integer newMasterEpoch, | |||
final Integer syncStateSetEpoch, final Set<Long> syncStateSet) { | |||
final Integer syncStateSetEpoch, final Set<Long> syncStateSet) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception
is too general...Create one within the project exception hierarchy
@@ -235,7 +235,7 @@ public synchronized void changeBrokerRole(final Long newMasterBrokerId, final St | |||
} | |||
} | |||
|
|||
public void changeToMaster(final int newMasterEpoch, final int syncStateSetEpoch, final Set<Long> syncStateSet) { | |||
public void changeToMaster(final int newMasterEpoch, final int syncStateSetEpoch, final Set<Long> syncStateSet) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please break this huge PR into smaller ones
OK, I will split it into two PRS, metadata and consumeQueue |
Which Issue(s) This PR Fixes
Fixes #7064
Brief Description
This proposal mainly optimizes and solves the existing perfermance problems in the million-topic scenario from two levels: