[GitHub] [carbondata] vikramahuja1001 opened a new pull request #4035: [WIP]: CleanFiles Behaviour Change
Locked
1234
1234
 
|
vikramahuja1001 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r535287432 ########## File path: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ########## @@ -1039,17 +1039,19 @@ private static void writeLoadMetadata(AbsoluteTableIdentifier identifier, } } - private static ReturnTuple isUpdateRequired(boolean isForceDeletion, CarbonTable carbonTable, - AbsoluteTableIdentifier absoluteTableIdentifier, LoadMetadataDetails[] details) { + private static ReturnTuple isUpdateRequired(boolean cleanStaleInProgress, boolean Review comment: i have put it back ########## File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java ########## @@ -173,40 +176,68 @@ public boolean accept(CarbonFile file) { } private static boolean checkIfLoadCanBeDeleted(LoadMetadataDetails oneLoad, - boolean isForceDelete) { - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() || - SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad.getSegmentStatus()) - && oneLoad.getVisibility().equalsIgnoreCase("true")) { - if (isForceDelete) { - return true; - } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); + boolean cleanStaleInProgress, boolean cleanCompactedAndMFD) { + if (oneLoad.getVisibility().equalsIgnoreCase("true")) { Review comment: it's handeled in checkIfLoadCanBeDeletedPhysically method, it does not check visibility before deleting ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonCleanFilesCommand.scala ########## @@ -117,9 +119,9 @@ case class CarbonCleanFilesCommand( CleanFilesUtil.cleanStaleSegments(carbonTable) } if (forceTableClean) { Review comment: it is removed in the PR4013 by David ########## File path: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ########## @@ -1039,17 +1039,19 @@ private static void writeLoadMetadata(AbsoluteTableIdentifier identifier, } } - private static ReturnTuple isUpdateRequired(boolean isForceDeletion, CarbonTable carbonTable, - AbsoluteTableIdentifier absoluteTableIdentifier, LoadMetadataDetails[] details) { + private static ReturnTuple isUpdateRequired(boolean cleanStaleInProgress, boolean Review comment: updated as per the above discussion ########## File path: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ########## @@ -1039,17 +1039,19 @@ private static void writeLoadMetadata(AbsoluteTableIdentifier identifier, } } - private static ReturnTuple isUpdateRequired(boolean isForceDeletion, CarbonTable carbonTable, Review comment: done ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
vikramahuja1001 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r535465503 ########## File path: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ########## @@ -1095,7 +1099,8 @@ public static void deleteLoadsAndUpdateMetadata(CarbonTable carbonTable, boolean // if execute command 'clean files' or the number of invisible segment info // exceeds the value of 'carbon.invisible.segments.preserve.count', // it need to append the invisible segment list to 'tablestatus.history' file. - if (isForceDeletion || (invisibleSegmentCnt > invisibleSegmentPreserveCnt)) { + if (cleanStaleInprogress || cleanCompactedAndMFD || (invisibleSegmentCnt > Review comment: changes it to isCleanFilesOperation, if we do not keep this, we will change the behaviour of show history segments, as that file would never be updated, since previously clean files was always force option, it would always go in that if condition for clean 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738280044 ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738624282 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/5061/ ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738625173 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3302/ ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
ajantha-bhat commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r536052356 ########## File path: docs/clean-files.md ########## @@ -24,6 +24,7 @@ Clean files command is used to remove the Compacted, Marked For Delete ,In Progr ``` CLEAN FILES FOR TABLE TABLE_NAME ``` +The above clean files command will clean Marked For Delete and Compacted segments depending on ```MAX_QUERY_EXECUTION_TIME``` (default 1 hr) and ``` carbon.trash.retention.days``` (default 7 days). It will also delete the timestamp subdirectories from the trash folder after expiration day(default 7 day, can be configured) Review comment: ```suggestion The above clean files command will clean Marked For Delete and Compacted segments depending on ```max.query.execution.time``` (default 1 hr) and ``` carbon.trash.retention.days``` (default 7 days). It will also delete the timestamp subdirectories from the trash folder after expiration day(default 7 day, can be configured) ``` ########## File path: integration/spark/src/test/scala/org/apache/spark/util/CarbonCommandSuite.scala ########## @@ -117,7 +117,7 @@ class CarbonCommandSuite extends QueryTest with BeforeAndAfterAll { val table = "carbon_table3" createAndLoadTestTable(table, "csv_table") DeleteSegmentById.main(Array(s"${location}", table, "0")) - CleanFiles.main(Array(s"${location}", table)) + CleanFiles.main(Array(s"${location}", table, "false", "true", "true")) Review comment: How it is passing without setting force allow carbon property ########## File path: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ########## @@ -1039,17 +999,19 @@ private static void writeLoadMetadata(AbsoluteTableIdentifier identifier, } } - private static ReturnTuple isUpdateRequired(boolean isForceDeletion, CarbonTable carbonTable, - AbsoluteTableIdentifier absoluteTableIdentifier, LoadMetadataDetails[] details) { + private static ReturnTuple isUpdateRequired(boolean isForceDeletion, boolean Review comment: please keep new argument in the end ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
vikramahuja1001 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r536082283 ########## File path: integration/spark/src/test/scala/org/apache/spark/util/CarbonCommandSuite.scala ########## @@ -117,7 +117,7 @@ class CarbonCommandSuite extends QueryTest with BeforeAndAfterAll { val table = "carbon_table3" createAndLoadTestTable(table, "csv_table") DeleteSegmentById.main(Array(s"${location}", table, "0")) - CleanFiles.main(Array(s"${location}", table)) + CleanFiles.main(Array(s"${location}", table, "false", "true", "true")) Review comment: added code to fail in the cleanFiles API also ########## File path: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ########## @@ -1039,17 +999,19 @@ private static void writeLoadMetadata(AbsoluteTableIdentifier identifier, } } - private static ReturnTuple isUpdateRequired(boolean isForceDeletion, CarbonTable carbonTable, - AbsoluteTableIdentifier absoluteTableIdentifier, LoadMetadataDetails[] details) { + private static ReturnTuple isUpdateRequired(boolean isForceDeletion, boolean Review comment: done ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
vikramahuja1001 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r536082456 ########## File path: docs/clean-files.md ########## @@ -24,6 +24,7 @@ Clean files command is used to remove the Compacted, Marked For Delete ,In Progr ``` CLEAN FILES FOR TABLE TABLE_NAME ``` +The above clean files command will clean Marked For Delete and Compacted segments depending on ```MAX_QUERY_EXECUTION_TIME``` (default 1 hr) and ``` carbon.trash.retention.days``` (default 7 days). It will also delete the timestamp subdirectories from the trash folder after expiration day(default 7 day, can be configured) Review comment: done ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738829218 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/5072/ ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738829559 Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3314/ ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
ajantha-bhat commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738862791 retest this please ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
ajantha-bhat commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738865456 LGTM ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738918591 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/5076/ ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-738923606 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3318/ ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
akashrn5 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r535840999 ########## File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java ########## @@ -173,40 +176,66 @@ public boolean accept(CarbonFile file) { } private static boolean checkIfLoadCanBeDeleted(LoadMetadataDetails oneLoad, - boolean isForceDelete) { - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() || - SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad.getSegmentStatus()) - && oneLoad.getVisibility().equalsIgnoreCase("true")) { - if (isForceDelete) { - return true; - } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); + boolean isForceDelete, boolean cleanStaleInProgress) { + if (oneLoad.getVisibility().equalsIgnoreCase("true")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); } - return false; } private static boolean checkIfLoadCanBeDeletedPhysically(LoadMetadataDetails oneLoad, - boolean isForceDelete) { + boolean isForceDelete, boolean cleanStaleInProgress) { // Check if the segment is added externally and path is set then do not delete it - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() - || SegmentStatus.COMPACTED == oneLoad.getSegmentStatus()) && (oneLoad.getPath() == null - || oneLoad.getPath().equalsIgnoreCase("NA"))) { - if (isForceDelete) { + if (oneLoad.getPath() == null || oneLoad.getPath().equalsIgnoreCase("NA")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); + } + return false; + } + + private static Boolean canDeleteThisLoad(LoadMetadataDetails oneLoad, + boolean isForceDelete, boolean cleanStaleInProgress) { + /* + * if cleanStaleInProgress == false and force == false, clean MFD and Compacted Review comment: ```suggestion * if cleanStaleInProgress == false and isForceDelete== false, clean MFD and Compacted ``` please handle in other comments ########## File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java ########## @@ -173,40 +176,66 @@ public boolean accept(CarbonFile file) { } private static boolean checkIfLoadCanBeDeleted(LoadMetadataDetails oneLoad, - boolean isForceDelete) { - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() || - SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad.getSegmentStatus()) - && oneLoad.getVisibility().equalsIgnoreCase("true")) { - if (isForceDelete) { - return true; - } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); + boolean isForceDelete, boolean cleanStaleInProgress) { + if (oneLoad.getVisibility().equalsIgnoreCase("true")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); } - return false; } private static boolean checkIfLoadCanBeDeletedPhysically(LoadMetadataDetails oneLoad, - boolean isForceDelete) { + boolean isForceDelete, boolean cleanStaleInProgress) { // Check if the segment is added externally and path is set then do not delete it - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() - || SegmentStatus.COMPACTED == oneLoad.getSegmentStatus()) && (oneLoad.getPath() == null - || oneLoad.getPath().equalsIgnoreCase("NA"))) { - if (isForceDelete) { + if (oneLoad.getPath() == null || oneLoad.getPath().equalsIgnoreCase("NA")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); + } + return false; + } + + private static Boolean canDeleteThisLoad(LoadMetadataDetails oneLoad, + boolean isForceDelete, boolean cleanStaleInProgress) { + /* + * if cleanStaleInProgress == false and force == false, clean MFD and Compacted + * segments will depend on query timeout(1 hr) and trashRetentionTimeout(7 days, default). + * For example: + * If trashRetentionTimeout is 7 days and query timeout is 1 hr--> Delete after 7 days + * If trashRetentionTimeout is 0 days and query timeout is 1 hr--> Delete after 1 hr + * + * if cleanStaleInProgress == false and force == true, clean MFD and Compacted + * segments immediately(Do not check for any timeout) + * + * if cleanStaleInProgress == true and force == false, clean Stale Inprogress, MFD and + * compacted segments after 7 days(taking carbon.trash.retention.time value) + * + * if cleanStaleInProgress == true and force == true, clean MFD, Compacted and + * stale inprogress segments immediately.(Do not check for any timeout) + */ + if (isForceDelete) { + // immediately delete compacted and MFD + if (SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || SegmentStatus + .MARKED_FOR_DELETE == oneLoad.getSegmentStatus()) { return true; } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - + // immediately delete inprogress segments if cleanstaleinprogress is true + return cleanStaleInProgress && (SegmentStatus.INSERT_IN_PROGRESS == oneLoad + .getSegmentStatus() || SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad + .getSegmentStatus()); + } + long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); + // in case there is no deletion or modification timestamp, take the load start time as + // deleteTime + if (deletionTime == 0) { + deletionTime = oneLoad.getLoadStartTime(); + } + if (SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || SegmentStatus + .MARKED_FOR_DELETE == oneLoad.getSegmentStatus()) { + // delete MFD, compacted segments after checking trash timeout and query timeout return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); - + .isMaxQueryTimeoutExceeded(deletionTime); } - - return false; + return (SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || SegmentStatus Review comment: where is the handling of the discussion, like, when any clean files called, we should check the trash folder and based on timeout we should delete it? ########## File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java ########## @@ -173,40 +176,66 @@ public boolean accept(CarbonFile file) { } private static boolean checkIfLoadCanBeDeleted(LoadMetadataDetails oneLoad, - boolean isForceDelete) { - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() || - SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad.getSegmentStatus()) - && oneLoad.getVisibility().equalsIgnoreCase("true")) { - if (isForceDelete) { - return true; - } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); + boolean isForceDelete, boolean cleanStaleInProgress) { + if (oneLoad.getVisibility().equalsIgnoreCase("true")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); } - return false; } private static boolean checkIfLoadCanBeDeletedPhysically(LoadMetadataDetails oneLoad, - boolean isForceDelete) { + boolean isForceDelete, boolean cleanStaleInProgress) { // Check if the segment is added externally and path is set then do not delete it - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() - || SegmentStatus.COMPACTED == oneLoad.getSegmentStatus()) && (oneLoad.getPath() == null - || oneLoad.getPath().equalsIgnoreCase("NA"))) { - if (isForceDelete) { + if (oneLoad.getPath() == null || oneLoad.getPath().equalsIgnoreCase("NA")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); + } + return false; + } + + private static Boolean canDeleteThisLoad(LoadMetadataDetails oneLoad, + boolean isForceDelete, boolean cleanStaleInProgress) { + /* + * if cleanStaleInProgress == false and force == false, clean MFD and Compacted + * segments will depend on query timeout(1 hr) and trashRetentionTimeout(7 days, default). + * For example: + * If trashRetentionTimeout is 7 days and query timeout is 1 hr--> Delete after 7 days + * If trashRetentionTimeout is 0 days and query timeout is 1 hr--> Delete after 1 hr + * + * if cleanStaleInProgress == false and force == true, clean MFD and Compacted + * segments immediately(Do not check for any timeout) + * + * if cleanStaleInProgress == true and force == false, clean Stale Inprogress, MFD and + * compacted segments after 7 days(taking carbon.trash.retention.time value) + * + * if cleanStaleInProgress == true and force == true, clean MFD, Compacted and + * stale inprogress segments immediately.(Do not check for any timeout) + */ + if (isForceDelete) { + // immediately delete compacted and MFD + if (SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || SegmentStatus + .MARKED_FOR_DELETE == oneLoad.getSegmentStatus()) { return true; } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - + // immediately delete inprogress segments if cleanstaleinprogress is true + return cleanStaleInProgress && (SegmentStatus.INSERT_IN_PROGRESS == oneLoad + .getSegmentStatus() || SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad + .getSegmentStatus()); + } + long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); Review comment: you can combine from 224 to 229 to conditional statement ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
vikramahuja1001 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r536540410 ########## File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java ########## @@ -173,40 +176,66 @@ public boolean accept(CarbonFile file) { } private static boolean checkIfLoadCanBeDeleted(LoadMetadataDetails oneLoad, - boolean isForceDelete) { - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() || - SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad.getSegmentStatus()) - && oneLoad.getVisibility().equalsIgnoreCase("true")) { - if (isForceDelete) { - return true; - } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); + boolean isForceDelete, boolean cleanStaleInProgress) { + if (oneLoad.getVisibility().equalsIgnoreCase("true")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); } - return false; } private static boolean checkIfLoadCanBeDeletedPhysically(LoadMetadataDetails oneLoad, - boolean isForceDelete) { + boolean isForceDelete, boolean cleanStaleInProgress) { // Check if the segment is added externally and path is set then do not delete it - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() - || SegmentStatus.COMPACTED == oneLoad.getSegmentStatus()) && (oneLoad.getPath() == null - || oneLoad.getPath().equalsIgnoreCase("NA"))) { - if (isForceDelete) { + if (oneLoad.getPath() == null || oneLoad.getPath().equalsIgnoreCase("NA")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); + } + return false; + } + + private static Boolean canDeleteThisLoad(LoadMetadataDetails oneLoad, + boolean isForceDelete, boolean cleanStaleInProgress) { + /* + * if cleanStaleInProgress == false and force == false, clean MFD and Compacted + * segments will depend on query timeout(1 hr) and trashRetentionTimeout(7 days, default). + * For example: + * If trashRetentionTimeout is 7 days and query timeout is 1 hr--> Delete after 7 days + * If trashRetentionTimeout is 0 days and query timeout is 1 hr--> Delete after 1 hr + * + * if cleanStaleInProgress == false and force == true, clean MFD and Compacted + * segments immediately(Do not check for any timeout) + * + * if cleanStaleInProgress == true and force == false, clean Stale Inprogress, MFD and + * compacted segments after 7 days(taking carbon.trash.retention.time value) + * + * if cleanStaleInProgress == true and force == true, clean MFD, Compacted and + * stale inprogress segments immediately.(Do not check for any timeout) + */ + if (isForceDelete) { + // immediately delete compacted and MFD + if (SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || SegmentStatus + .MARKED_FOR_DELETE == oneLoad.getSegmentStatus()) { return true; } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - + // immediately delete inprogress segments if cleanstaleinprogress is true + return cleanStaleInProgress && (SegmentStatus.INSERT_IN_PROGRESS == oneLoad + .getSegmentStatus() || SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad + .getSegmentStatus()); + } + long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); + // in case there is no deletion or modification timestamp, take the load start time as + // deleteTime + if (deletionTime == 0) { + deletionTime = oneLoad.getLoadStartTime(); + } + if (SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || SegmentStatus + .MARKED_FOR_DELETE == oneLoad.getSegmentStatus()) { + // delete MFD, compacted segments after checking trash timeout and query timeout return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); - + .isMaxQueryTimeoutExceeded(deletionTime); } - - return false; + return (SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || SegmentStatus Review comment: that is happening in another class(CarbonCleanFilesCommand). ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
vikramahuja1001 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r536540531 ########## File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java ########## @@ -173,40 +176,66 @@ public boolean accept(CarbonFile file) { } private static boolean checkIfLoadCanBeDeleted(LoadMetadataDetails oneLoad, - boolean isForceDelete) { - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() || - SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad.getSegmentStatus()) - && oneLoad.getVisibility().equalsIgnoreCase("true")) { - if (isForceDelete) { - return true; - } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); + boolean isForceDelete, boolean cleanStaleInProgress) { + if (oneLoad.getVisibility().equalsIgnoreCase("true")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); } - return false; } private static boolean checkIfLoadCanBeDeletedPhysically(LoadMetadataDetails oneLoad, - boolean isForceDelete) { + boolean isForceDelete, boolean cleanStaleInProgress) { // Check if the segment is added externally and path is set then do not delete it - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() - || SegmentStatus.COMPACTED == oneLoad.getSegmentStatus()) && (oneLoad.getPath() == null - || oneLoad.getPath().equalsIgnoreCase("NA"))) { - if (isForceDelete) { + if (oneLoad.getPath() == null || oneLoad.getPath().equalsIgnoreCase("NA")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); + } + return false; + } + + private static Boolean canDeleteThisLoad(LoadMetadataDetails oneLoad, + boolean isForceDelete, boolean cleanStaleInProgress) { + /* + * if cleanStaleInProgress == false and force == false, clean MFD and Compacted Review comment: done ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
vikramahuja1001 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r536540616 ########## File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java ########## @@ -173,40 +176,66 @@ public boolean accept(CarbonFile file) { } private static boolean checkIfLoadCanBeDeleted(LoadMetadataDetails oneLoad, - boolean isForceDelete) { - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() || - SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad.getSegmentStatus()) - && oneLoad.getVisibility().equalsIgnoreCase("true")) { - if (isForceDelete) { - return true; - } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); + boolean isForceDelete, boolean cleanStaleInProgress) { + if (oneLoad.getVisibility().equalsIgnoreCase("true")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); } - return false; } private static boolean checkIfLoadCanBeDeletedPhysically(LoadMetadataDetails oneLoad, - boolean isForceDelete) { + boolean isForceDelete, boolean cleanStaleInProgress) { // Check if the segment is added externally and path is set then do not delete it - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() - || SegmentStatus.COMPACTED == oneLoad.getSegmentStatus()) && (oneLoad.getPath() == null - || oneLoad.getPath().equalsIgnoreCase("NA"))) { - if (isForceDelete) { + if (oneLoad.getPath() == null || oneLoad.getPath().equalsIgnoreCase("NA")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); + } + return false; + } + + private static Boolean canDeleteThisLoad(LoadMetadataDetails oneLoad, + boolean isForceDelete, boolean cleanStaleInProgress) { + /* + * if cleanStaleInProgress == false and force == false, clean MFD and Compacted + * segments will depend on query timeout(1 hr) and trashRetentionTimeout(7 days, default). + * For example: + * If trashRetentionTimeout is 7 days and query timeout is 1 hr--> Delete after 7 days + * If trashRetentionTimeout is 0 days and query timeout is 1 hr--> Delete after 1 hr + * + * if cleanStaleInProgress == false and force == true, clean MFD and Compacted + * segments immediately(Do not check for any timeout) + * + * if cleanStaleInProgress == true and force == false, clean Stale Inprogress, MFD and + * compacted segments after 7 days(taking carbon.trash.retention.time value) + * + * if cleanStaleInProgress == true and force == true, clean MFD, Compacted and + * stale inprogress segments immediately.(Do not check for any timeout) + */ + if (isForceDelete) { + // immediately delete compacted and MFD + if (SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || SegmentStatus + .MARKED_FOR_DELETE == oneLoad.getSegmentStatus()) { return true; } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - + // immediately delete inprogress segments if cleanstaleinprogress is true + return cleanStaleInProgress && (SegmentStatus.INSERT_IN_PROGRESS == oneLoad + .getSegmentStatus() || SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad + .getSegmentStatus()); + } + long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); Review comment: The deletion time is used later also in line 238, can not combine it ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
vikramahuja1001 commented on a change in pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#discussion_r536542022 ########## File path: core/src/main/java/org/apache/carbondata/core/util/DeleteLoadFolders.java ########## @@ -173,40 +176,66 @@ public boolean accept(CarbonFile file) { } private static boolean checkIfLoadCanBeDeleted(LoadMetadataDetails oneLoad, - boolean isForceDelete) { - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() || - SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || - SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad.getSegmentStatus()) - && oneLoad.getVisibility().equalsIgnoreCase("true")) { - if (isForceDelete) { - return true; - } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); + boolean isForceDelete, boolean cleanStaleInProgress) { + if (oneLoad.getVisibility().equalsIgnoreCase("true")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); } - return false; } private static boolean checkIfLoadCanBeDeletedPhysically(LoadMetadataDetails oneLoad, - boolean isForceDelete) { + boolean isForceDelete, boolean cleanStaleInProgress) { // Check if the segment is added externally and path is set then do not delete it - if ((SegmentStatus.MARKED_FOR_DELETE == oneLoad.getSegmentStatus() - || SegmentStatus.COMPACTED == oneLoad.getSegmentStatus()) && (oneLoad.getPath() == null - || oneLoad.getPath().equalsIgnoreCase("NA"))) { - if (isForceDelete) { + if (oneLoad.getPath() == null || oneLoad.getPath().equalsIgnoreCase("NA")) { + return canDeleteThisLoad(oneLoad, isForceDelete, cleanStaleInProgress); + } + return false; + } + + private static Boolean canDeleteThisLoad(LoadMetadataDetails oneLoad, + boolean isForceDelete, boolean cleanStaleInProgress) { + /* + * if cleanStaleInProgress == false and force == false, clean MFD and Compacted + * segments will depend on query timeout(1 hr) and trashRetentionTimeout(7 days, default). + * For example: + * If trashRetentionTimeout is 7 days and query timeout is 1 hr--> Delete after 7 days + * If trashRetentionTimeout is 0 days and query timeout is 1 hr--> Delete after 1 hr + * + * if cleanStaleInProgress == false and force == true, clean MFD and Compacted + * segments immediately(Do not check for any timeout) + * + * if cleanStaleInProgress == true and force == false, clean Stale Inprogress, MFD and + * compacted segments after 7 days(taking carbon.trash.retention.time value) + * + * if cleanStaleInProgress == true and force == true, clean MFD, Compacted and + * stale inprogress segments immediately.(Do not check for any timeout) + */ + if (isForceDelete) { + // immediately delete compacted and MFD + if (SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || SegmentStatus + .MARKED_FOR_DELETE == oneLoad.getSegmentStatus()) { return true; } - long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); - + // immediately delete inprogress segments if cleanstaleinprogress is true + return cleanStaleInProgress && (SegmentStatus.INSERT_IN_PROGRESS == oneLoad + .getSegmentStatus() || SegmentStatus.INSERT_OVERWRITE_IN_PROGRESS == oneLoad + .getSegmentStatus()); + } + long deletionTime = oneLoad.getModificationOrDeletionTimestamp(); + // in case there is no deletion or modification timestamp, take the load start time as + // deleteTime + if (deletionTime == 0) { + deletionTime = oneLoad.getLoadStartTime(); + } + if (SegmentStatus.COMPACTED == oneLoad.getSegmentStatus() || SegmentStatus + .MARKED_FOR_DELETE == oneLoad.getSegmentStatus()) { + // delete MFD, compacted segments after checking trash timeout and query timeout return TrashUtil.isTrashRetentionTimeoutExceeded(deletionTime) && CarbonUpdateUtil - .isMaxQueryTimeoutExceeded(deletionTime); - + .isMaxQueryTimeoutExceeded(deletionTime); } - - return false; + return (SegmentStatus.INSERT_IN_PROGRESS == oneLoad.getSegmentStatus() || SegmentStatus Review comment: CarbonCLeanFiles Command line 116 ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
|
|
In reply to this post by GitBox
akashrn5 commented on pull request #4035: URL: https://github.com/apache/carbondata/pull/4035#issuecomment-739144048 LGTM ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [hidden email] |
«
Return to Apache CarbonData JIRA issues
|
1 view|%1 views
| Free forum by Nabble | Edit this page |