-
Notifications
You must be signed in to change notification settings - Fork 11.9k
[ISSUE #3148]Support metadata export #3149
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
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3149 +/- ##
=============================================
+ Coverage 47.82% 48.05% +0.23%
+ Complexity 4551 4348 -203
=============================================
Files 552 514 -38
Lines 36643 35040 -1603
Branches 4846 4669 -177
=============================================
- Hits 17526 16840 -686
+ Misses 16881 16015 -866
+ Partials 2236 2185 -51
Continue to review full report at Codecov.
|
distribution/bin/export.sh
Outdated
download() { | ||
if [[ -e /tmp/rocketmq/ ]]; then | ||
rm -rf /tmp/rocketmq/ | ||
fi | ||
|
||
wget --no-check-certificate -P /tmp/rocketmq/ https://ons-migration.oss-cn-hangzhou.aliyuncs.com/rocketmq-for-export.tar.gz | ||
if [[ $? -ne 0 ]]; then | ||
echo -e "[ERROR] Download rocketmq error, please make sure this file exists" | ||
exit 1 | ||
else | ||
echo -e "[INFO] Download rocketmq completed,file path: $(pwd)/rocketmq-for-export.tar.gz" | ||
fi | ||
|
||
tar -zxvf /tmp/rocketmq/rocketmq-for-export.tar.gz -C /tmp/rocketmq/ > /dev/null 2>&1 | ||
|
||
echo -e "[INFO] Unzip rocketmq-for-export.tar.gz completed" | ||
} |
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 is not appropriate to download packages from OSS 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.
Any update for 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.
has been deleted about oss
|
||
try { | ||
String clusterName = commandLine.getOptionValue('c').trim(); | ||
String filePath = !commandLine.hasOption('f') ? "/tmp/rocketmq/export" : commandLine.getOptionValue('f') |
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.
Define a constant for "/tmp/rocketmq/export"
ConcurrentMap<String, TopicConfig> topicConfigMap = new ConcurrentHashMap<>(); | ||
ConcurrentMap<String, SubscriptionGroupConfig> subGroupConfigMap = new ConcurrentHashMap<>(); |
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 topicConfigMap
subGroupConfigMap
be used by multiple threads? If not, it would be better just use plain HashMap here.
Map<String, Properties> map = new HashMap<>(); | ||
Properties masterProperties = defaultMQAdminExt.getBrokerConfig(masterAddr); | ||
map.put("master", needBrokerProprties(masterProperties)); | ||
masterBrokerSize++; | ||
slaveBrokerSize += masterAndSlaveMap.get(masterAddr).size(); | ||
|
||
brokerConfigs.put(masterProperties.getProperty("brokerName"), needBrokerProprties(masterProperties)); |
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.
What is the purpose of map defined in this loop?
.iterator(); | ||
while (iterator.hasNext()) { | ||
String topic = iterator.next().getKey(); | ||
if (topicList.getTopicList().contains(topic) || !specialTopic && (topic.startsWith(MixAll.RETRY_GROUP_TOPIC_PREFIX) || topic.startsWith(MixAll.DLQ_GROUP_TOPIC_PREFIX))) { |
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.
!specialTopic && (topic.startsWith(MixAll.RETRY_GROUP_TOPIC_PREFIX) || topic.startsWith(MixAll.DLQ_GROUP_TOPIC_PREFIX))
这段逻辑应该再加个括号
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.
嗯嗯,逻辑清晰一点,未来加个条件也不会出bug
Make sure set the target branch to
develop
What is the purpose of the change
XXXXX
Brief changelog
XX
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily. Notice,
it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR
.[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.