[RIP-7] Fix some issue and Optimization code for multiple_disk_dir #1144

wangshaojie4039 · 2019-04-08T09:16:50Z

What is the purpose of the change

Add Multiple Directories Storage Support for commit log.
Detailed info is in RIP-7

Brief changelog

(1).Check whether the disk is full and available before creating the file
(2).When deleting files, change the calculation method of disk remaining space
(3).Support for online write-off paths

Verifying this change

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.

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

zongtanghu · 2019-04-09T02:35:17Z

store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java

@@ -114,7 +113,7 @@
    boolean shutDownNormal = false;

    public DefaultMessageStore(final MessageStoreConfig messageStoreConfig, final BrokerStatsManager brokerStatsManager,
-        final MessageArrivingListener messageArrivingListener, final BrokerConfig brokerConfig) throws IOException {
+                               final MessageArrivingListener messageArrivingListener, final BrokerConfig brokerConfig) throws IOException {


Here,you should make sure your code style has no issues.

zongtanghu · 2019-04-09T02:35:31Z

store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java

-        final int maxMsgNums,
-        final MessageFilter messageFilter) {
+                                       final int maxMsgNums,
+                                       final MessageFilter messageFilter) {


Here,as above!

zongtanghu · 2019-04-09T02:37:25Z

common/src/main/java/org/apache/rocketmq/common/UtilAll.java

@@ -199,18 +199,20 @@ public static double getDiskPartitionSpaceUsedPercent(final String path) {
        if (null == path || path.isEmpty())
            return -1;

+        String[] pathArr = path.trim().split(";");
        try {


In the function,you should add some comments,such as if you path parameter is a mutiple-disk paths variable.

zongtanghu · 2019-04-09T02:43:59Z

store/src/main/java/org/apache/rocketmq/store/MultiPathMappedFileQueue.java

    private final MessageStoreConfig config;

    public MultiPathMappedFileQueue(MessageStoreConfig messageStoreConfig, int mappedFileSize,
-        AllocateMappedFileService allocateMappedFileService) {
+                                    AllocateMappedFileService allocateMappedFileService) {
        super(messageStoreConfig.getStorePathCommitLog(), mappedFileSize, allocateMappedFileService);


Here,you should make sure your code style has no issues as above!

zongtanghu · 2019-04-09T03:25:11Z

common/src/main/java/org/apache/rocketmq/common/UtilAll.java

                long usedSpace = totalSpace - freeSpace;
-
                return usedSpace / (double) totalSpace;
            }
        } catch (Exception e) {


Here,you can print the exception error info by using printStackTrace

Jason918 · 2019-04-15T03:01:46Z

store/src/main/java/org/apache/rocketmq/store/config/MessageStoreConfig.java

@@ -36,9 +33,9 @@

    private boolean multiCommitLogPathEnable = false;

-    private List<String> commitLogStorePaths = null;
+    private String commitLogStorePaths = null;


Why change List to Sting ? Repeatedly parse this parameter every u need use it.

Jason918 · 2019-04-15T03:14:05Z

common/src/main/java/org/apache/rocketmq/common/UtilAll.java

@@ -199,18 +199,20 @@ public static double getDiskPartitionSpaceUsedPercent(final String path) {
        if (null == path || path.isEmpty())
            return -1;

+        String[] pathArr = path.trim().split(";");


IMHO, It's better to put all these parse actions in one place.

Jason918 · 2019-04-15T03:19:02Z

store/src/main/java/org/apache/rocketmq/store/MultiPathMappedFileQueue.java

+    public String[] getCommitLogStorePaths() {
+        String[] storPaths = null;
+        if (!UtilAll.isBlank(config.getCommitLogStorePaths())) {
+            storPaths = config.getCommitLogStorePaths().trim().split(";") ;


';' is really not a shell friendly file path separator. IMHO, ',' or ':' is more commonly used.

liuruiyiyang · 2019-07-24T09:15:12Z

store/src/test/java/org/apache/rocketmq/store/MultiPathMappedFileQueueTest.java

@@ -83,18 +84,18 @@ public void testUpdatePathsOnline() {

        MessageStoreConfig config = new MessageStoreConfig();
        config.setMultiCommitLogPathEnable(true);
-        config.setCommitLogStorePaths("target/unit_test_store/a/:target/unit_test_store/b/:target/unit_test_store/c/");
-        MappedFileQueue mappedFileQueue = new MultiPathMappedFileQueue(config, 1024, null);
+        config.setCommitLogStorePaths("target/unit_test_store/a/;target/unit_test_store/b/;target/unit_test_store/c/");


"," is more commonly used as the path separator in config parameters. IMHO, I suggest using "," instead of ";" as the path separator.

vongosling · 2019-07-24T11:41:05Z

store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java

-                        }
+                double physicRatio = UtilAll.getDiskPartitionSpaceUsedPercent(storePaths);
+                if (physicRatio > diskSpaceWarningLevelRatio) {
+                    boolean diskok = DefaultMessageStore.this.runningFlags.getAndMakeDiskFull();


Any update? what's the meaning of the diskok?

zongtanghu · 2019-11-28T02:31:25Z

@vongosling @Jason918 @liuruiyiyang @duhenglucky @RongtongJin please help to review this pr together.Thanks

zongtanghu · 2019-11-28T02:34:49Z

common/src/main/java/org/apache/rocketmq/common/UtilAll.java

+//    public static double getMaxDiskPartitionSpaceUsedPercent(final String paths) {
+//        if (null == paths || paths.isEmpty()) {
+//            return -1;
+//        }
+//
+//        String[] pathArr = paths.trim().split(";");
+//        try {
+//
+//            double maxUsedPercent = 0;
+//            for (int i = 0; i < pathArr.length; i++) {
+//                File file = new File(pathArr[i]);
+//                if (!file.exists()) {
+//                    continue;
+//                }
+//                long totalSpace = file.getTotalSpace();
+//                long freeSpace = file.getFreeSpace();
+//                double usedPercent = (totalSpace - freeSpace) / (double) totalSpace;
+//                if (usedPercent > maxUsedPercent) {
+//                    maxUsedPercent = usedPercent;
+//                }
+//            }
+//            return maxUsedPercent;
+//        } catch (Exception e) {
+//            return -1;
+//        }
+//    }
+
+
+
+


Why do you add code annotation in this place.

If this method isn't necessary,you can remove it! @wangshaojie4039

Jason918 · 2021-11-07T03:53:17Z

I am closing this as RIP-7 was merge with another PR #3357

[RIP-7] fix some issue and Optimization code

a93b9e2

zongtanghu changed the title ~~[RIP-7] fix some issue and Optimization code~~ [RIP-7] fix some issue and Optimization code for multiple_disk_dir Apr 8, 2019

zongtanghu self-requested a review April 8, 2019 10:03

zongtanghu added the type/enhancement label Apr 8, 2019

zongtanghu added this to the 4.6.0 milestone Apr 8, 2019

zongtanghu approved these changes Apr 9, 2019

View reviewed changes

vongosling changed the title ~~[RIP-7] fix some issue and Optimization code for multiple_disk_dir~~ [RIP-7] Fix some issue and Optimization code for multiple_disk_dir Apr 12, 2019

Jason918 reviewed Apr 15, 2019

View reviewed changes

liuruiyiyang reviewed Jul 24, 2019

View reviewed changes

vongosling reviewed Jul 24, 2019

View reviewed changes

wangshaojie4039 added 2 commits November 25, 2019 17:40

fix some bugs

c326a3c

fix some bugs

651fd42

duhenglucky modified the milestones: 4.6.0, 4.7.0 Nov 27, 2019

zongtanghu reviewed Nov 28, 2019

View reviewed changes

ShannonDing modified the milestones: 4.7.0, 4.7.1 Mar 30, 2020

RongtongJin modified the milestones: 4.7.1, 4.8.0 May 25, 2020

RongtongJin modified the milestones: 4.8.0, 4.8.1 Dec 8, 2020

Jason918 closed this Nov 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RIP-7] Fix some issue and Optimization code for multiple_disk_dir #1144

[RIP-7] Fix some issue and Optimization code for multiple_disk_dir #1144

wangshaojie4039 commented Apr 8, 2019

zongtanghu Apr 9, 2019

zongtanghu Apr 9, 2019

zongtanghu Apr 9, 2019

zongtanghu Apr 9, 2019

zongtanghu Apr 9, 2019

Jason918 Apr 15, 2019

Jason918 Apr 15, 2019

Jason918 Apr 15, 2019

liuruiyiyang Jul 24, 2019

vongosling Jul 24, 2019

zongtanghu commented Nov 28, 2019

zongtanghu Nov 28, 2019

zongtanghu Nov 28, 2019

Jason918 commented Nov 7, 2021

[RIP-7] Fix some issue and Optimization code for multiple_disk_dir #1144

[RIP-7] Fix some issue and Optimization code for multiple_disk_dir #1144

Conversation

wangshaojie4039 commented Apr 8, 2019

What is the purpose of the change

Brief changelog

Verifying this change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zongtanghu commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jason918 commented Nov 7, 2021