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 #8058] Support for upgrading metadata in json to rocksdb (#8045) #8116

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

LetLetMe
Copy link
Contributor

@LetLetMe LetLetMe commented May 10, 2024

Which Issue(s) This PR Fixes

Fixes #8058

Brief Description

How Did You Test This Change?

@LetLetMe LetLetMe force-pushed the develop branch 4 times, most recently from f762936 to 2bf3840 Compare May 11, 2024 03:06
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2024

Codecov Report

Attention: Patch coverage is 54.46009% with 97 lines in your changes are missing coverage. Please review.

Project coverage is 43.00%. Comparing base (159a603) to head (6c4186e).
Report is 11 commits behind head on develop.

Files Patch % Lines
...s/command/metadata/RocksDBConfigToJsonCommand.java 0.00% 30 Missing ⚠️
.../subscription/RocksDBSubscriptionGroupManager.java 68.35% 18 Missing and 7 partials ⚠️
...e/rocketmq/common/config/ConfigRocksDBStorage.java 0.00% 16 Missing ⚠️
...cketmq/broker/topic/RocksDBTopicConfigManager.java 71.42% 5 Missing and 5 partials ⚠️
...ache/rocketmq/broker/topic/TopicConfigManager.java 58.33% 5 Missing ⚠️
...g/apache/rocketmq/broker/RocksDBConfigManager.java 86.36% 2 Missing and 1 partial ⚠️
...ache/rocketmq/store/config/MessageStoreConfig.java 25.00% 3 Missing ⚠️
...mq/broker/offset/RocksDBConsumerOffsetManager.java 50.00% 1 Missing and 1 partial ⚠️
.../broker/subscription/SubscriptionGroupManager.java 80.00% 2 Missing ⚠️
.../apache/rocketmq/tools/command/MQAdminStartup.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #8116      +/-   ##
=============================================
+ Coverage      42.94%   43.00%   +0.06%     
- Complexity     10387    10412      +25     
=============================================
  Files           1270     1270              
  Lines          88694    88828     +134     
  Branches       11401    11404       +3     
=============================================
+ Hits           38092    38204     +112     
+ Misses         45914    45902      -12     
- Partials        4688     4722      +34     

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

@LetLetMe LetLetMe force-pushed the develop branch 2 times, most recently from 363759e to f718207 Compare May 11, 2024 06:56
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.

I checked the code, and it seems that upgrading from the 5.2.0 version with RocksDB enabled to this modified version is fully compatible, isn't it?

@LetLetMe
Copy link
Contributor Author

I checked the code, and it seems that upgrading from the 5.2.0 version with RocksDB enabled to this modified version is fully compatible, isn't it?

没问题
No problem

@LetLetMe LetLetMe changed the title [ISSUE #8058]Support for upgrading metadata in json to rocksdb (#8045) [ISSUE #8058] Support for upgrading metadata in json to rocksdb (#8045) May 22, 2024
this.configRocksDBStorage = new ConfigRocksDBStorage(filePath);
return this.configRocksDBStorage.start();
}
public final boolean loadDataVersion() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use public final for method here?

kvDataVersion = StringUtils.isNotBlank(currDataVersionString) ? JSON.parseObject(currDataVersionString, DataVersion.class) : new DataVersion();
return true;
} catch (Exception e) {
throw new RuntimeException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about print some information for the exception here?

return true;
}

public boolean loadSubscriptionGroupAndForbidden() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to do as a pipeline case here , use if-else may be a more readable way.

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 for upgrading metadata in json to rocksdb
5 participants