[CARBONDATA-4081] Fix multiple issues with clean files command #4051

vikramahuja1001 · 2020-12-10T09:12:15Z

Why is this PR needed?

While getting stale segment, we do a list files and take all the files, if there is any folder/file other than .segment file, it will lead to further issues while copying data to the trash folder.
In the case when AbstractDFSCarbonFile is created with path and HadoopConf instead of fileStatus and if the file does not exist, since fileStatus is empty ListDirs returns empty result and getAbsolutePath throws file does not exist exception
Clean files is not allowed with concurrent insert overwrite in progress, In case with concurrent loading to MV is by default an insert overwrite operation, clean files operation on that MV would fail and throw exception.

What changes were proposed in this PR?

Added a filter, to only consider the files ending with ".segment"
Using listfiles instead of list dirs in the trash folder
No need to throw exceptions, just put a info message in case of such MV tables and continue blocking clean files for such MV.

Does this PR introduce any user interface change?

No

Is any new testcase added?

No

CarbonDataQA2 · 2020-12-10T10:49:22Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5137/

CarbonDataQA2 · 2020-12-10T11:09:09Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3375/

QiangCai · 2020-12-11T01:34:47Z

can you add a test case for fault testing?

vikramahuja1001 · 2020-12-15T10:13:03Z

@QiangCai , i have changed current test cases itself. Please check

CarbonDataQA2 · 2020-12-15T11:48:43Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5166/

CarbonDataQA2 · 2020-12-15T11:54:10Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3404/

fileStatus is null

CarbonDataQA2 · 2020-12-15T14:09:11Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5168/

CarbonDataQA2 · 2020-12-15T14:09:43Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3406/

ajantha-bhat · 2020-12-15T14:16:32Z

@vikramahuja1001 : when the issue is found, what other file was present in segment metadata directory ? how it is impacting ?

…ite in progress

vikramahuja1001 · 2020-12-16T06:15:02Z

@ajantha-bhat , so in the case of partition table, we have a tmp directory in the segment folder. In this case, while we check for stale segments, we are blindly reading all the files in the segment folder and collecting the name of it and then based on that we read the segment files, since it is not a file, thus it cannot be read and would give an exception. In this fix, we just need to filter if the file ends with ".segment"

Indhumathi27 · 2020-12-16T07:17:38Z

core/src/main/java/org/apache/carbondata/core/util/TrashUtil.java

@@ -157,11 +157,12 @@ public static void deleteExpiredDataFromTrash(String tablePath) {
    // Deleting the timestamp based subdirectories in the trashfolder by the given timestamp.
    try {
      if (FileFactory.isFileExist(trashPath)) {


Suggested change

if (FileFactory.isFileExist(trashPath)) {

CarbonFile trashFile = FileFactory.getCarbonFile(trashPath);

if (trashFile.exists()) {

CarbonFile[] timestampFolderList = trashFile.listFiles();

I think we should deprecate listfiles and fileExist and other API in file factory that takes path, it has to take carbon file instead of path. This way we can force user to think about reusing the carbonFile Object.
we can raise a JIRA and assign some new contributors.

currently Vikram can handle for his PR by accepting suggested changes by Indhu. But many places we are not resuing the CarbonFile object. that can be handled by my above point.

https://issues.apache.org/jira/browse/CARBONDATA-4086

@ajantha-bhat , i can raise a PR for that as well

Indhumathi27 · 2020-12-16T07:18:21Z

core/src/main/java/org/apache/carbondata/core/util/TrashUtil.java

@@ -181,7 +182,7 @@ public static void emptyTrash(String tablePath) {
    // if the trash folder exists delete the contents of the trash folder
    try {
      if (FileFactory.isFileExist(trashPath)) {


handle same as above comment

Indhumathi27 · 2020-12-16T07:24:19Z

...c/main/scala/org/apache/spark/sql/execution/command/management/CarbonCleanFilesCommand.scala

+          CarbonFilters.getPartitions(Seq.empty[Expression], sparkSession, carbonTable))
+      }
+    } else {
+      LOGGER.info(s"Can not do clean files operation for the MV: ${carbonTable.getTableName}")


Clean files for MV is not supported?

changed code

please handle based on isInternalCleanCall

CarbonDataQA2 · 2020-12-16T07:55:18Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5175/

CarbonDataQA2 · 2020-12-16T07:58:47Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3413/

CarbonDataQA2 · 2020-12-16T11:45:54Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5182/

CarbonDataQA2 · 2020-12-16T11:46:41Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3420/

ajantha-bhat · 2020-12-17T06:00:31Z

...c/main/scala/org/apache/spark/sql/execution/command/management/CarbonCleanFilesCommand.scala

-    // if insert overwrite in progress, do not allow delete segment
-    if (SegmentStatusManager.isOverwriteInProgressInTable(carbonTable)) {
+    // if insert overwrite in progress and table not a MV, do not allow delete segment
+    if (!carbonTable.isMV && SegmentStatusManager.isOverwriteInProgressInTable(carbonTable)) {


Not here, handle at the place where we call clean files for MV when cleanfiles is called for maintable. Else if the user calls clean files on MV table when concurrently insert overwrite is happening, now you don't throw an exception. which is out of synch with main table clean files behavior.

please handle based on isInternalCleanCall

ajantha-bhat · 2020-12-17T06:01:05Z

...c/main/scala/org/apache/spark/sql/execution/command/management/CarbonCleanFilesCommand.scala

  override def processData(sparkSession: SparkSession): Seq[Row] = {
    Checker.validateTableExists(databaseNameOp, tableName, sparkSession)
    val carbonTable = CarbonEnv.getCarbonTable(databaseNameOp, tableName)(sparkSession)
    setAuditTable(carbonTable)
-    // if insert overwrite in progress, do not allow delete segment
-    if (SegmentStatusManager.isOverwriteInProgressInTable(carbonTable)) {
+    // if insert overwrite in progress and table not a MV, do not allow delete segment


Suggested change

// if insert overwrite in progress and table not a MV, do not allow delete segment

// if insert overwrite in progress and table is not a MV, do not allow delete segment

CarbonDataQA2 · 2020-12-17T10:09:17Z

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3434/

CarbonDataQA2 · 2020-12-17T10:10:00Z

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5194/

ajantha-bhat · 2020-12-17T11:14:02Z

LGTM

vikramahuja1001 changed the title ~~[WIP] Only consider .segment files for stale segments~~ [CARBONDATA-4081] In Clean files operation, only consider ".segment" files in the segments dir for cleaning stale segments Dec 10, 2020

vikramahuja1001 changed the title ~~[CARBONDATA-4081] In Clean files operation, only consider ".segment" files in the segments dir for cleaning stale segments~~ [CARBONDATA-4081] In Clean files operation, only consider ".segment" files in the segments folder for cleaning stale segments Dec 10, 2020

Only consider .segment files for stale segments

f87b3a5

vikramahuja1001 force-pushed the oldstoreFix branch from 9853b3d to f87b3a5 Compare December 15, 2020 10:12

Using listfiles instead of listDirs, as listDirs do not work when

a5661d0

fileStatus is null

vikramahuja1001 changed the title ~~[CARBONDATA-4081] In Clean files operation, only consider ".segment" files in the segments folder for cleaning stale segments~~ [CARBONDATA-4081] Fix multiple issues with clean files command Dec 15, 2020

Do not throw exception for clean files on mv table with insert overwr…

fff268e

…ite in progress

vikramahuja1001 force-pushed the oldstoreFix branch from 6090eda to fff268e Compare December 16, 2020 06:07

Indhumathi27 reviewed Dec 16, 2020

View reviewed changes

Issue fixes

1310929

ajantha-bhat reviewed Dec 17, 2020

View reviewed changes

fixes

50d12b9

asfgit closed this in 7aafb6b Dec 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CARBONDATA-4081] Fix multiple issues with clean files command #4051

[CARBONDATA-4081] Fix multiple issues with clean files command #4051

vikramahuja1001 commented Dec 10, 2020 •

edited

CarbonDataQA2 commented Dec 10, 2020

CarbonDataQA2 commented Dec 10, 2020

QiangCai commented Dec 11, 2020

vikramahuja1001 commented Dec 15, 2020

CarbonDataQA2 commented Dec 15, 2020

CarbonDataQA2 commented Dec 15, 2020

CarbonDataQA2 commented Dec 15, 2020

CarbonDataQA2 commented Dec 15, 2020

ajantha-bhat commented Dec 15, 2020

vikramahuja1001 commented Dec 16, 2020

Indhumathi27 Dec 16, 2020

ajantha-bhat Dec 16, 2020

ajantha-bhat Dec 16, 2020

ajantha-bhat Dec 16, 2020

vikramahuja1001 Dec 16, 2020

vikramahuja1001 Dec 16, 2020

Indhumathi27 Dec 16, 2020

vikramahuja1001 Dec 16, 2020

Indhumathi27 Dec 16, 2020

vikramahuja1001 Dec 16, 2020

ajantha-bhat Dec 17, 2020

CarbonDataQA2 commented Dec 16, 2020

CarbonDataQA2 commented Dec 16, 2020

CarbonDataQA2 commented Dec 16, 2020

CarbonDataQA2 commented Dec 16, 2020

ajantha-bhat Dec 17, 2020

ajantha-bhat Dec 17, 2020

ajantha-bhat Dec 17, 2020

CarbonDataQA2 commented Dec 17, 2020

CarbonDataQA2 commented Dec 17, 2020

ajantha-bhat commented Dec 17, 2020

-      if (FileFactory.isFileExist(trashPath)) {
+      CarbonFile trashFile = FileFactory.getCarbonFile(trashPath);
+      if (trashFile.exists()) {
+        CarbonFile[] timestampFolderList = trashFile.listFiles();

	// if insert overwrite in progress and table not a MV, do not allow delete segment
	// if insert overwrite in progress and table is not a MV, do not allow delete segment

[CARBONDATA-4081] Fix multiple issues with clean files command #4051

[CARBONDATA-4081] Fix multiple issues with clean files command #4051

Conversation

vikramahuja1001 commented Dec 10, 2020 • edited

Why is this PR needed?

What changes were proposed in this PR?

Does this PR introduce any user interface change?

Is any new testcase added?

CarbonDataQA2 commented Dec 10, 2020

CarbonDataQA2 commented Dec 10, 2020

QiangCai commented Dec 11, 2020

vikramahuja1001 commented Dec 15, 2020

CarbonDataQA2 commented Dec 15, 2020

CarbonDataQA2 commented Dec 15, 2020

CarbonDataQA2 commented Dec 15, 2020

CarbonDataQA2 commented Dec 15, 2020

ajantha-bhat commented Dec 15, 2020

vikramahuja1001 commented Dec 16, 2020

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

Choose a reason for hiding this comment

CarbonDataQA2 commented Dec 16, 2020

CarbonDataQA2 commented Dec 16, 2020

CarbonDataQA2 commented Dec 16, 2020

CarbonDataQA2 commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA2 commented Dec 17, 2020

CarbonDataQA2 commented Dec 17, 2020

ajantha-bhat commented Dec 17, 2020

vikramahuja1001 commented Dec 10, 2020 •

edited