You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/02/10 10:12:43 UTC

[GitHub] [iotdb] choubenson opened a new pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

choubenson opened a new pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031


   After compaction, we have some operations as following:
   (1) move tmp target files to final target files
   (2) update memory, make it atomic.
   (3) delete source files and its corresponding files 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804398175



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/CrossSpaceCompactionExceptionHandler.java
##########
@@ -136,43 +127,77 @@ private static boolean handleWhenAllSourceFilesExist(
       List<TsFileResource> unseqFileList,
       TsFileManager tsFileManager)
       throws IOException {
-    for (TsFileResource seqFile : seqFileList) {
-      ModificationFile compactionModFile = ModificationFile.getCompactionMods(seqFile);
-      if (compactionModFile.exists()) {
-        Collection<Modification> modifications = compactionModFile.getModifications();
-        ModificationFile normalModification = ModificationFile.getNormalMods(seqFile);
-        for (Modification modification : modifications) {
-          normalModification.write(modification);
+    TsFileResourceList unseqTsFileResourceList =
+        tsFileManager.getUnsequenceListByTimePartition(unseqFileList.get(0).getTimePartition());
+    TsFileResourceList seqTsFileResourceList =
+        tsFileManager.getSequenceListByTimePartition(seqFileList.get(0).getTimePartition());
+
+    // append new modifications to old mods files
+    CompactionUtils.appendNewModificationsToOldModsFile(seqFileList);
+    CompactionUtils.appendNewModificationsToOldModsFile(unseqFileList);
+
+    boolean removeAllTargetFile = true;
+    try {
+      seqTsFileResourceList.writeLock();
+      unseqTsFileResourceList.writeLock();

Review comment:
       resolved




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] qiaojialin commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804517248



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileManager.java
##########
@@ -171,6 +172,52 @@ public void addAll(List<TsFileResource> tsFileResourceList, boolean sequence) {
     }
   }
 
+  /** This method is called after compaction to update memory. */
+  public void replace(
+      List<TsFileResource> seqFileResources,
+      List<TsFileResource> unseqFileResources,
+      List<TsFileResource> targetFileResources,
+      long timePartition)
+      throws IOException {
+    writeLock("replace");
+    try {
+      for (TsFileResource tsFileResource : seqFileResources) {
+        for (Map.Entry<Long, TsFileResourceList> entry : sequenceFiles.entrySet()) {
+          if (entry.getValue().contains(tsFileResource)) {

Review comment:
       TsFileResourceList.remove() already check this, no need to check

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileManager.java
##########
@@ -171,6 +172,52 @@ public void addAll(List<TsFileResource> tsFileResourceList, boolean sequence) {
     }
   }
 
+  /** This method is called after compaction to update memory. */
+  public void replace(
+      List<TsFileResource> seqFileResources,
+      List<TsFileResource> unseqFileResources,
+      List<TsFileResource> targetFileResources,
+      long timePartition)
+      throws IOException {
+    writeLock("replace");
+    try {
+      for (TsFileResource tsFileResource : seqFileResources) {
+        for (Map.Entry<Long, TsFileResourceList> entry : sequenceFiles.entrySet()) {
+          if (entry.getValue().contains(tsFileResource)) {
+            entry.getValue().remove(tsFileResource);
+            TsFileResourceManager.getInstance().removeTsFileResource(tsFileResource);
+            break;
+          }
+        }
+      }
+      for (TsFileResource tsFileResource : unseqFileResources) {
+        for (Map.Entry<Long, TsFileResourceList> entry : unsequenceFiles.entrySet()) {
+          if (entry.getValue().contains(tsFileResource)) {
+            entry.getValue().remove(tsFileResource);
+            TsFileResourceManager.getInstance().removeTsFileResource(tsFileResource);
+            break;
+          }
+        }
+      }
+      if (unseqFileResources.size() == 0 || seqFileResources.size() != 0) {

Review comment:
       add a parameter in this method to indicate where to insert the targetTsFileResource

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/rewrite/selector/RewriteCompactionFileSelector.java
##########
@@ -259,8 +259,12 @@ private void selectOverlappedSeqFiles(TsFileResource unseqFile) {
         long seqEndTime = seqFile.getEndTime(deviceId);
         long seqStartTime = seqFile.getStartTime(deviceId);
         if (unseqEndTime < seqStartTime) {
-          // if time range in unseq file is 10-20, seq file is 30-40, then skip this seq file and
-          // there is no more overlap later.
+          // if time range in unseq file is 10-20, seq file is 30-40, if this unseq file has no
+          // overlapped seq files, then select this seq file or skip this seq file and there is no
+          // more overlap later.

Review comment:
       ```suggestion
             // Suppose the time range in unseq file is 10-20, seq file is 30-40, if this unseq file has no
             // overlapped seq files, then select this seq file and there is no
             // more overlap later.
   ```

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/rewrite/task/RewriteCrossSpaceCompactionTask.java
##########
@@ -172,18 +177,11 @@ private void executeCompaction()
   }
 
   private void updateTsFileResource() throws IOException {

Review comment:
       As there is only one caller, remove this method.

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileManager.java
##########
@@ -171,6 +172,52 @@ public void addAll(List<TsFileResource> tsFileResourceList, boolean sequence) {
     }
   }
 
+  /** This method is called after compaction to update memory. */
+  public void replace(
+      List<TsFileResource> seqFileResources,
+      List<TsFileResource> unseqFileResources,
+      List<TsFileResource> targetFileResources,
+      long timePartition)
+      throws IOException {
+    writeLock("replace");
+    try {
+      for (TsFileResource tsFileResource : seqFileResources) {
+        for (Map.Entry<Long, TsFileResourceList> entry : sequenceFiles.entrySet()) {
+          if (entry.getValue().contains(tsFileResource)) {
+            entry.getValue().remove(tsFileResource);
+            TsFileResourceManager.getInstance().removeTsFileResource(tsFileResource);

Review comment:
       ```suggestion
               if (entry.getValue().remove(tsFileResource)) {
                           TsFileResourceManager.getInstance().removeTsFileResource(tsFileResource);
               }
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804352153



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/CrossSpaceCompactionExceptionHandler.java
##########
@@ -136,43 +127,77 @@ private static boolean handleWhenAllSourceFilesExist(
       List<TsFileResource> unseqFileList,
       TsFileManager tsFileManager)
       throws IOException {
-    for (TsFileResource seqFile : seqFileList) {
-      ModificationFile compactionModFile = ModificationFile.getCompactionMods(seqFile);
-      if (compactionModFile.exists()) {
-        Collection<Modification> modifications = compactionModFile.getModifications();
-        ModificationFile normalModification = ModificationFile.getNormalMods(seqFile);
-        for (Modification modification : modifications) {
-          normalModification.write(modification);
+    TsFileResourceList unseqTsFileResourceList =
+        tsFileManager.getUnsequenceListByTimePartition(unseqFileList.get(0).getTimePartition());

Review comment:
       resolved
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804576680



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/rewrite/selector/RewriteCompactionFileSelector.java
##########
@@ -259,8 +259,12 @@ private void selectOverlappedSeqFiles(TsFileResource unseqFile) {
         long seqEndTime = seqFile.getEndTime(deviceId);
         long seqStartTime = seqFile.getStartTime(deviceId);
         if (unseqEndTime < seqStartTime) {
-          // if time range in unseq file is 10-20, seq file is 30-40, then skip this seq file and
-          // there is no more overlap later.
+          // if time range in unseq file is 10-20, seq file is 30-40, if this unseq file has no
+          // overlapped seq files, then select this seq file or skip this seq file and there is no
+          // more overlap later.

Review comment:
       resolved




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804575722



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileManager.java
##########
@@ -171,6 +172,52 @@ public void addAll(List<TsFileResource> tsFileResourceList, boolean sequence) {
     }
   }
 
+  /** This method is called after compaction to update memory. */
+  public void replace(
+      List<TsFileResource> seqFileResources,
+      List<TsFileResource> unseqFileResources,
+      List<TsFileResource> targetFileResources,
+      long timePartition)
+      throws IOException {
+    writeLock("replace");
+    try {
+      for (TsFileResource tsFileResource : seqFileResources) {
+        for (Map.Entry<Long, TsFileResourceList> entry : sequenceFiles.entrySet()) {
+          if (entry.getValue().contains(tsFileResource)) {
+            entry.getValue().remove(tsFileResource);
+            TsFileResourceManager.getInstance().removeTsFileResource(tsFileResource);
+            break;
+          }
+        }
+      }
+      for (TsFileResource tsFileResource : unseqFileResources) {
+        for (Map.Entry<Long, TsFileResourceList> entry : unsequenceFiles.entrySet()) {
+          if (entry.getValue().contains(tsFileResource)) {
+            entry.getValue().remove(tsFileResource);
+            TsFileResourceManager.getInstance().removeTsFileResource(tsFileResource);
+            break;
+          }
+        }
+      }
+      if (unseqFileResources.size() == 0 || seqFileResources.size() != 0) {

Review comment:
       resolved




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] coveralls edited a comment on pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#issuecomment-1034794787


   
   [![Coverage Status](https://coveralls.io/builds/46446096/badge)](https://coveralls.io/builds/46446096)
   
   Coverage increased (+0.002%) to 67.724% when pulling **ee7dd748fb58b0c5464f3807ec2472cf0b06cb24 on choubenson:changeCompaction** into **38eaf0ebd51b250ba6e66cb93c25680574d8c5dc on apache:master**.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] coveralls edited a comment on pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#issuecomment-1034794787


   
   [![Coverage Status](https://coveralls.io/builds/46457025/badge)](https://coveralls.io/builds/46457025)
   
   Coverage decreased (-0.008%) to 67.714% when pulling **3a8baa8602e729ffd40a2e84651b8075f5b3ac08 on choubenson:changeCompaction** into **38eaf0ebd51b250ba6e66cb93c25680574d8c5dc on apache:master**.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804578824



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/rewrite/task/RewriteCrossSpaceCompactionTask.java
##########
@@ -172,18 +177,11 @@ private void executeCompaction()
   }
 
   private void updateTsFileResource() throws IOException {

Review comment:
       resolved




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804575538



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -359,6 +359,22 @@ private static void updateOneTargetMods(
     targetFile.getModFile().close();
   }
 
+  public static void removeCompactionModification(
+      List<TsFileResource> selectedSeqTsFileResourceList,
+      List<TsFileResource> selectedUnSeqTsFileResourceList,
+      String fullStorageGroupName) {
+    try {
+      for (TsFileResource seqFile : selectedSeqTsFileResourceList) {
+        ModificationFile.getCompactionMods(seqFile).remove();

Review comment:
       resolved
   

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileManager.java
##########
@@ -171,6 +172,52 @@ public void addAll(List<TsFileResource> tsFileResourceList, boolean sequence) {
     }
   }
 
+  /** This method is called after compaction to update memory. */
+  public void replace(
+      List<TsFileResource> seqFileResources,
+      List<TsFileResource> unseqFileResources,
+      List<TsFileResource> targetFileResources,
+      long timePartition)
+      throws IOException {
+    writeLock("replace");
+    try {
+      for (TsFileResource tsFileResource : seqFileResources) {
+        for (Map.Entry<Long, TsFileResourceList> entry : sequenceFiles.entrySet()) {
+          if (entry.getValue().contains(tsFileResource)) {

Review comment:
       resolved




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804398089



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/rewrite/task/RewriteCrossSpaceCompactionTask.java
##########
@@ -172,18 +177,22 @@ private void executeCompaction()
   }
 
   private void updateTsFileResource() throws IOException {
+    seqTsFileResourceList.writeLock();
+    unseqTsFileResourceList.writeLock();

Review comment:
       resolved




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] qiaojialin merged pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
qiaojialin merged pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r804578603



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileManager.java
##########
@@ -171,6 +172,52 @@ public void addAll(List<TsFileResource> tsFileResourceList, boolean sequence) {
     }
   }
 
+  /** This method is called after compaction to update memory. */
+  public void replace(
+      List<TsFileResource> seqFileResources,
+      List<TsFileResource> unseqFileResources,
+      List<TsFileResource> targetFileResources,
+      long timePartition)
+      throws IOException {
+    writeLock("replace");
+    try {
+      for (TsFileResource tsFileResource : seqFileResources) {
+        for (Map.Entry<Long, TsFileResourceList> entry : sequenceFiles.entrySet()) {
+          if (entry.getValue().contains(tsFileResource)) {
+            entry.getValue().remove(tsFileResource);
+            TsFileResourceManager.getInstance().removeTsFileResource(tsFileResource);

Review comment:
       resolved




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] coveralls commented on pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#issuecomment-1034794787


   
   [![Coverage Status](https://coveralls.io/builds/46410356/badge)](https://coveralls.io/builds/46410356)
   
   Coverage increased (+0.01%) to 67.737% when pulling **ec89ebbb511005b7e2a92bb495ba3511b28079c2 on choubenson:changeCompaction** into **38eaf0ebd51b250ba6e66cb93c25680574d8c5dc on apache:master**.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] choubenson commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
choubenson commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r803589581



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -359,6 +359,22 @@ private static void updateOneTargetMods(
     targetFile.getModFile().close();
   }
 
+  public static void removeCompactionModification(
+      List<TsFileResource> selectedSeqTsFileResourceList,
+      List<TsFileResource> selectedUnSeqTsFileResourceList,
+      String fullStorageGroupName) {
+    try {
+      for (TsFileResource seqFile : selectedSeqTsFileResourceList) {
+        ModificationFile.getCompactionMods(seqFile).remove();
+      }
+      for (TsFileResource unseqFile : selectedUnSeqTsFileResourceList) {
+        ModificationFile.getCompactionMods(unseqFile).remove();
+      }
+    } catch (IOException e) {
+      logger.error("{} cannot remove merging modification ", fullStorageGroupName, e);

Review comment:
       resolved
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] coveralls edited a comment on pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#issuecomment-1034794787


   
   [![Coverage Status](https://coveralls.io/builds/46452038/badge)](https://coveralls.io/builds/46452038)
   
   Coverage increased (+0.02%) to 67.743% when pulling **61576893144ce8ea37104ade6b711921edd39c36 on choubenson:changeCompaction** into **38eaf0ebd51b250ba6e66cb93c25680574d8c5dc on apache:master**.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] coveralls edited a comment on pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#issuecomment-1034794787


   
   [![Coverage Status](https://coveralls.io/builds/46456006/badge)](https://coveralls.io/builds/46456006)
   
   Coverage decreased (-0.02%) to 67.705% when pulling **3a8baa8602e729ffd40a2e84651b8075f5b3ac08 on choubenson:changeCompaction** into **38eaf0ebd51b250ba6e66cb93c25680574d8c5dc on apache:master**.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [iotdb] qiaojialin commented on a change in pull request #5031: [IOTDB-2500] [ query & cross-compaction ] cross-compaction stuck

Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #5031:
URL: https://github.com/apache/iotdb/pull/5031#discussion_r803518125



##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -359,6 +359,22 @@ private static void updateOneTargetMods(
     targetFile.getModFile().close();
   }
 
+  public static void removeCompactionModification(
+      List<TsFileResource> selectedSeqTsFileResourceList,
+      List<TsFileResource> selectedUnSeqTsFileResourceList,
+      String fullStorageGroupName) {
+    try {
+      for (TsFileResource seqFile : selectedSeqTsFileResourceList) {
+        ModificationFile.getCompactionMods(seqFile).remove();
+      }
+      for (TsFileResource unseqFile : selectedUnSeqTsFileResourceList) {
+        ModificationFile.getCompactionMods(unseqFile).remove();
+      }
+    } catch (IOException e) {
+      logger.error("{} cannot remove merging modification ", fullStorageGroupName, e);

Review comment:
       ```suggestion
         logger.error("{} cannot remove compaction modification ", fullStorageGroupName, e);
   ```
   
   Also print the mod file name

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/CrossSpaceCompactionExceptionHandler.java
##########
@@ -136,43 +127,77 @@ private static boolean handleWhenAllSourceFilesExist(
       List<TsFileResource> unseqFileList,
       TsFileManager tsFileManager)
       throws IOException {
-    for (TsFileResource seqFile : seqFileList) {
-      ModificationFile compactionModFile = ModificationFile.getCompactionMods(seqFile);
-      if (compactionModFile.exists()) {
-        Collection<Modification> modifications = compactionModFile.getModifications();
-        ModificationFile normalModification = ModificationFile.getNormalMods(seqFile);
-        for (Modification modification : modifications) {
-          normalModification.write(modification);
+    TsFileResourceList unseqTsFileResourceList =
+        tsFileManager.getUnsequenceListByTimePartition(unseqFileList.get(0).getTimePartition());

Review comment:
       How about passing the time partition in this method

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/rewrite/task/RewriteCrossSpaceCompactionTask.java
##########
@@ -172,18 +177,22 @@ private void executeCompaction()
   }
 
   private void updateTsFileResource() throws IOException {
+    seqTsFileResourceList.writeLock();
+    unseqTsFileResourceList.writeLock();

Review comment:
       when we change the resourceList, we need to get the writelock of TsFileManager, not the two list. 
   The query only get the read lock of TsFileManager

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/cross/CrossSpaceCompactionExceptionHandler.java
##########
@@ -136,43 +127,77 @@ private static boolean handleWhenAllSourceFilesExist(
       List<TsFileResource> unseqFileList,
       TsFileManager tsFileManager)
       throws IOException {
-    for (TsFileResource seqFile : seqFileList) {
-      ModificationFile compactionModFile = ModificationFile.getCompactionMods(seqFile);
-      if (compactionModFile.exists()) {
-        Collection<Modification> modifications = compactionModFile.getModifications();
-        ModificationFile normalModification = ModificationFile.getNormalMods(seqFile);
-        for (Modification modification : modifications) {
-          normalModification.write(modification);
+    TsFileResourceList unseqTsFileResourceList =
+        tsFileManager.getUnsequenceListByTimePartition(unseqFileList.get(0).getTimePartition());
+    TsFileResourceList seqTsFileResourceList =
+        tsFileManager.getSequenceListByTimePartition(seqFileList.get(0).getTimePartition());
+
+    // append new modifications to old mods files
+    CompactionUtils.appendNewModificationsToOldModsFile(seqFileList);
+    CompactionUtils.appendNewModificationsToOldModsFile(unseqFileList);
+
+    boolean removeAllTargetFile = true;
+    try {
+      seqTsFileResourceList.writeLock();
+      unseqTsFileResourceList.writeLock();

Review comment:
       get the write lock of TsFileManager

##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -359,6 +359,22 @@ private static void updateOneTargetMods(
     targetFile.getModFile().close();
   }
 
+  public static void removeCompactionModification(
+      List<TsFileResource> selectedSeqTsFileResourceList,
+      List<TsFileResource> selectedUnSeqTsFileResourceList,
+      String fullStorageGroupName) {
+    try {
+      for (TsFileResource seqFile : selectedSeqTsFileResourceList) {
+        ModificationFile.getCompactionMods(seqFile).remove();

Review comment:
       How about making getCompactionMods() as a method of TsFileResource




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org