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 #8589] Support file format CQ and json format offset in-place upgrade to rocksdb management #8600

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

LetLetMe
Copy link
Contributor

Which Issue(s) This PR Fixes

Fixes #8589

Brief Description

How Did You Test This Change?

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 24.36364% with 208 lines in your changes missing coverage. Please review.

Project coverage is 47.40%. Comparing base (cab4fda) to head (7c87d9f).

Files with missing lines Patch % Lines
...ocketmq/broker/processor/AdminBrokerProcessor.java 0.00% 61 Missing ⚠️
...mand/queue/CheckRocksdbCqWriteProgressCommand.java 0.00% 38 Missing ⚠️
...org/apache/rocketmq/store/RocksDBMessageStore.java 0.00% 18 Missing ⚠️
...mq/broker/offset/RocksDBConsumerOffsetManager.java 52.94% 10 Missing and 6 partials ⚠️
...org/apache/rocketmq/store/DefaultMessageStore.java 33.33% 11 Missing and 1 partial ⚠️
...g/apache/rocketmq/client/impl/MQClientAPIImpl.java 0.00% 8 Missing ⚠️
...ache/rocketmq/store/config/MessageStoreConfig.java 41.66% 7 Missing ⚠️
.../subscription/RocksDBSubscriptionGroupManager.java 53.84% 4 Missing and 2 partials ⚠️
...cketmq/broker/topic/RocksDBTopicConfigManager.java 50.00% 4 Missing and 2 partials ⚠️
.../rocketmq/broker/offset/ConsumerOffsetManager.java 58.33% 3 Missing and 2 partials ⚠️
... and 11 more
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #8600      +/-   ##
=============================================
- Coverage      47.52%   47.40%   -0.13%     
- Complexity     11548    11558      +10     
=============================================
  Files           1279     1282       +3     
  Lines          89558    89808     +250     
  Branches       11514    11551      +37     
=============================================
+ Hits           42563    42571       +8     
- Misses         41778    41996     +218     
- Partials        5217     5241      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@lizhanhui lizhanhui left a comment

Choose a reason for hiding this comment

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

Moving offset management to RocksDB is very good to have. but decisions and rational should be explained.

@LetLetMe
Copy link
Contributor Author

LetLetMe commented Sep 3, 2024

Moving offset management to RocksDB is very good to have. but decisions and rational should be explained.

我们rocksdb方案中,topic,group,offset,cq都是存储在rocksdb中,前三者我们认为都是元数据

@LetLetMe
Copy link
Contributor Author

Moving offset management to RocksDB is very good to have. but decisions and rational should be explained.

我们的rocksdb方案 offset已经是rocksdb存储的了,不是这里引入的

@lizhanhui
Copy link
Contributor

lizhanhui commented Sep 13, 2024

There is another issue to address: file backed ConsumeQueue now unconditionally preserve the last file in the file queue. If configured to RocksDB only, need to clean them up once the file-based CQ segments are totally expired.

@lizhanhui
Copy link
Contributor

As previously mentioned, overall it is good to have PR, but there are many issues to resolve yet.

@lizhanhui
Copy link
Contributor

lizhanhui commented Sep 13, 2024

Moving offset management to RocksDB is very good to have. but decisions and rational should be explained.

我们的rocksdb方案 offset已经是rocksdb存储的了,不是这里引入的

It's NOT about introducing RocksDB, it's about decisions how to migrate from file-based one to the KV counterpart.

@LetLetMe
Copy link
Contributor Author

LetLetMe commented Sep 14, 2024

Moving offset management to RocksDB is very good to have. but decisions and rational should be explained.

我们的rocksdb方案 offset已经是rocksdb存储的了,不是这里引入的

It's NOT about introducing RocksDB, it's about decisions how to migrate from file-based one to the KV counterpart.

和元数据(topic,group)一样,在启动的时候会校验DataVersion,然后Transfer一次

Like the metadata (topic, group), DataVersion will be verified upon startup, and then transferred once.

@lizhanhui
Copy link
Contributor

Got another issue, if rocksdbCQDoubleWriteEnable is on, meaning users still have chance to rollback to file-based CQ and offset management, offsets should also write in dual manner.

This PR currently only write to KV.

@LetLetMe
Copy link
Contributor Author

Got another issue, if rocksdbCQDoubleWriteEnable is on, meaning users still have chance to rollback to file-based CQ and offset management, offsets should also write in dual manner.

This PR currently only write to KV.

#8699
#8698
这俩个额外的issue会被用来关联这些问题,我会在接下来光速修复掉
These two additional issues will be used to associate these problems, and I will fix them at lightning speed.

lizhanhui
lizhanhui previously approved these changes Sep 14, 2024
Copy link
Contributor

@lizhanhui lizhanhui left a comment

Choose a reason for hiding this comment

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

Look forward to see this awesome feature in prod with following issues resolved. Nice job overall.

@lizhimins lizhimins reopened this Sep 20, 2024
Copy link
Contributor

@fuyou001 fuyou001 left a comment

Choose a reason for hiding this comment

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

Waiting for code review

Copy link
Contributor

@fuyou001 fuyou001 left a comment

Choose a reason for hiding this comment

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

LGTM

@lizhimins lizhimins changed the title [Enhancement] Support JSON CQ Files and offset In-place Upgrade to RocksDB [ISSUE ##8589] Support file format CQ and json format offset manager in-place upgrade to RocksDB. Sep 23, 2024
@lizhimins lizhimins changed the title [ISSUE ##8589] Support file format CQ and json format offset manager in-place upgrade to RocksDB. [ISSUE #8589] Support file format cq and json format offset manager in-place upgrade to RocksDB. Sep 23, 2024
@lizhimins lizhimins changed the title [ISSUE #8589] Support file format cq and json format offset manager in-place upgrade to RocksDB. [ISSUE #8589] Support file format CQ and json format offset in-place upgrade to RocksDB. Sep 23, 2024
@lizhimins lizhimins changed the title [ISSUE #8589] Support file format CQ and json format offset in-place upgrade to RocksDB. [ISSUE #8589] Support file format CQ and json format offset in-place upgrade to rocksdb management Sep 23, 2024
@lizhimins lizhimins merged commit 525f877 into apache:develop Sep 23, 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.

[Enhancement] Support JSON CQ Files and offset In-place Upgrade to RocksDB
7 participants