You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2020/10/16 10:31:17 UTC
[GitHub] [hive] vpnvishv opened a new pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
vpnvishv opened a new pull request #1583:
URL: https://github.com/apache/hive/pull/1583
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Added support to delay cleanup of obsolete files/dirs with user configured value.
I have just ported this change from Qubole's hive-3 branch to master with some changes, it still requires thorough review wrt to Master branch. In this change we have reused CQ_META_INFO field from COMPACTION_QUEUE to store compaction end time. As we didn't wanted to change schema in branch-3. This can be changed in master by adding a new field in compaction_queue.
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
added unit test
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvargacl commented on a change in pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
Posted by GitBox <gi...@apache.org>.
pvargacl commented on a change in pull request #1583:
URL: https://github.com/apache/hive/pull/1583#discussion_r509327437
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -237,10 +238,22 @@ public void markCompacted(CompactionInfo info) throws MetaException {
try {
dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
stmt = dbConn.createStatement();
- String s = "UPDATE \"COMPACTION_QUEUE\" SET \"CQ_STATE\" = '" + READY_FOR_CLEANING + "', "
+ long now = getDbTime(dbConn);
+ String s = "UPDATE \"COMPACTION_QUEUE\" SET \"CQ_META_INFO\" = " + now + ", \"CQ_STATE\" = '" + READY_FOR_CLEANING + "', "
Review comment:
You should use a new field for that, and also can leverage TxnDbUtil.getEpochFn
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] github-actions[bot] commented on pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1583:
URL: https://github.com/apache/hive/pull/1583#issuecomment-748703248
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvargacl commented on a change in pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
Posted by GitBox <gi...@apache.org>.
pvargacl commented on a change in pull request #1583:
URL: https://github.com/apache/hive/pull/1583#discussion_r509315841
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
##########
@@ -244,7 +249,32 @@ private void removeFiles(String location, ValidWriteIdList writeIdList, Compacti
obsoleteDirs.addAll(dir.getAbortedDirectories());
List<Path> filesToDelete = new ArrayList<>(obsoleteDirs.size());
StringBuilder extraDebugInfo = new StringBuilder("[");
+ boolean delayedCleanupEnabled = conf.getBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_DELAYED_CLEANUP_ENABLED);
+
for (Path stat : obsoleteDirs) {
+ if (delayedCleanupEnabled) {
+ String filename = stat.toString();
+ if (filename.startsWith(AcidUtils.BASE_PREFIX)) {
+ long writeId = AcidUtils.ParsedBase.parseBase(stat).getWriteId();
+ if (ci.type == CompactionType.MINOR) {
+ LOG.info("Skipping base dir " + stat + " as this cleanup is for minor compaction"
+ + ", compaction id " + ci.id);
+ continue;
+ } else if (writeId > writeIdList.getHighWatermark()) {
+ LOG.info("Skipping base dir " + stat + " deletion as WriteId of this base dir is"
+ + " greater than highWaterMark for compaction id " + ci.id);
+ continue;
+ }
+ }
+ else if (filename.startsWith(AcidUtils.DELTA_PREFIX) || filename.startsWith(AcidUtils.DELETE_DELTA_PREFIX)) {
+ AcidUtils.ParsedDelta delta = AcidUtils.parsedDelta(stat, fs);
Review comment:
There is a ParsedDeltaLight in AcidUtils, that is a cheaper way to parse out the maxWriteId. This parsedDelta method will issue a FS call to check if the delta is in raw format
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvargacl commented on a change in pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
Posted by GitBox <gi...@apache.org>.
pvargacl commented on a change in pull request #1583:
URL: https://github.com/apache/hive/pull/1583#discussion_r509324748
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
##########
@@ -229,6 +233,7 @@ private static String idWatermark(CompactionInfo ci) {
private void removeFiles(String location, ValidWriteIdList writeIdList, CompactionInfo ci)
throws IOException, NoSuchObjectException, MetaException {
Path locPath = new Path(location);
+ FileSystem fs = locPath.getFileSystem(conf);
Review comment:
This fs can passed to getAcidState
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvargacl commented on a change in pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
Posted by GitBox <gi...@apache.org>.
pvargacl commented on a change in pull request #1583:
URL: https://github.com/apache/hive/pull/1583#discussion_r509330534
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
##########
@@ -272,4 +302,22 @@ private void removeFiles(String location, ValidWriteIdList writeIdList, Compacti
fs.delete(dead, true);
}
}
+
+ /**
+ * Check if user configured retention time for the cleanup of obsolete directories/files for the table
+ * has passed or not
+ *
+ * @param ci CompactionInfo
+ * @return True, if retention time has passed and it is ok to clean, else false
+ */
+ public boolean isReadyToCleanWithRetentionPolicy(CompactionInfo ci) {
Review comment:
This whole thing could be added to the findReadyToClean's sql query where clause. It could use the TxnDbUtil.getEpochFn so there would be no need for new fields in CompactionInfo
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] github-actions[bot] closed pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #1583:
URL: https://github.com/apache/hive/pull/1583
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvargacl commented on a change in pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
Posted by GitBox <gi...@apache.org>.
pvargacl commented on a change in pull request #1583:
URL: https://github.com/apache/hive/pull/1583#discussion_r509322335
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
##########
@@ -244,7 +249,32 @@ private void removeFiles(String location, ValidWriteIdList writeIdList, Compacti
obsoleteDirs.addAll(dir.getAbortedDirectories());
List<Path> filesToDelete = new ArrayList<>(obsoleteDirs.size());
StringBuilder extraDebugInfo = new StringBuilder("[");
+ boolean delayedCleanupEnabled = conf.getBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_DELAYED_CLEANUP_ENABLED);
+
for (Path stat : obsoleteDirs) {
+ if (delayedCleanupEnabled) {
+ String filename = stat.toString();
+ if (filename.startsWith(AcidUtils.BASE_PREFIX)) {
+ long writeId = AcidUtils.ParsedBase.parseBase(stat).getWriteId();
+ if (ci.type == CompactionType.MINOR) {
+ LOG.info("Skipping base dir " + stat + " as this cleanup is for minor compaction"
+ + ", compaction id " + ci.id);
+ continue;
+ } else if (writeId > writeIdList.getHighWatermark()) {
+ LOG.info("Skipping base dir " + stat + " deletion as WriteId of this base dir is"
+ + " greater than highWaterMark for compaction id " + ci.id);
+ continue;
+ }
+ }
+ else if (filename.startsWith(AcidUtils.DELTA_PREFIX) || filename.startsWith(AcidUtils.DELETE_DELTA_PREFIX)) {
+ AcidUtils.ParsedDelta delta = AcidUtils.parsedDelta(stat, fs);
+ if (delta.getMaxWriteId() > writeIdList.getHighWatermark()) {
Review comment:
I am not sure about this check. I guess this is here, to prepare for the case when there were two compaction, and we are doing the cleanup of the first one and don't want to clean up the stuff that was compacted by the second one. But the cleaner validWriteId list is topped by the minOpenTxnId, so if everything was committed the writeIdList.getHighWatermark() will be NEXT_WRITE_ID -1 , so it won't prevent the cleaning of the second compacted stuff. Maybe you can use the highestWriteId from the CompactionInfo? Not sure.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1583: HIVE-24275: [DRAFT] adding support for delayed cleanup of obsolete files/dirs
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1583:
URL: https://github.com/apache/hive/pull/1583#discussion_r509643679
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
##########
@@ -244,7 +249,32 @@ private void removeFiles(String location, ValidWriteIdList writeIdList, Compacti
obsoleteDirs.addAll(dir.getAbortedDirectories());
List<Path> filesToDelete = new ArrayList<>(obsoleteDirs.size());
StringBuilder extraDebugInfo = new StringBuilder("[");
+ boolean delayedCleanupEnabled = conf.getBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_DELAYED_CLEANUP_ENABLED);
+
for (Path stat : obsoleteDirs) {
+ if (delayedCleanupEnabled) {
+ String filename = stat.toString();
+ if (filename.startsWith(AcidUtils.BASE_PREFIX)) {
+ long writeId = AcidUtils.ParsedBase.parseBase(stat).getWriteId();
+ if (ci.type == CompactionType.MINOR) {
+ LOG.info("Skipping base dir " + stat + " as this cleanup is for minor compaction"
+ + ", compaction id " + ci.id);
+ continue;
+ } else if (writeId > writeIdList.getHighWatermark()) {
+ LOG.info("Skipping base dir " + stat + " deletion as WriteId of this base dir is"
+ + " greater than highWaterMark for compaction id " + ci.id);
+ continue;
+ }
+ }
+ else if (filename.startsWith(AcidUtils.DELTA_PREFIX) || filename.startsWith(AcidUtils.DELETE_DELTA_PREFIX)) {
+ AcidUtils.ParsedDelta delta = AcidUtils.parsedDelta(stat, fs);
+ if (delta.getMaxWriteId() > writeIdList.getHighWatermark()) {
Review comment:
Please add relevant comments in the code, wherever its not very obvious.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java
##########
@@ -244,7 +249,32 @@ private void removeFiles(String location, ValidWriteIdList writeIdList, Compacti
obsoleteDirs.addAll(dir.getAbortedDirectories());
List<Path> filesToDelete = new ArrayList<>(obsoleteDirs.size());
StringBuilder extraDebugInfo = new StringBuilder("[");
+ boolean delayedCleanupEnabled = conf.getBoolVar(HiveConf.ConfVars.HIVE_COMPACTOR_DELAYED_CLEANUP_ENABLED);
+
for (Path stat : obsoleteDirs) {
+ if (delayedCleanupEnabled) {
+ String filename = stat.toString();
+ if (filename.startsWith(AcidUtils.BASE_PREFIX)) {
+ long writeId = AcidUtils.ParsedBase.parseBase(stat).getWriteId();
+ if (ci.type == CompactionType.MINOR) {
+ LOG.info("Skipping base dir " + stat + " as this cleanup is for minor compaction"
+ + ", compaction id " + ci.id);
+ continue;
+ } else if (writeId > writeIdList.getHighWatermark()) {
+ LOG.info("Skipping base dir " + stat + " deletion as WriteId of this base dir is"
+ + " greater than highWaterMark for compaction id " + ci.id);
+ continue;
+ }
+ }
+ else if (filename.startsWith(AcidUtils.DELTA_PREFIX) || filename.startsWith(AcidUtils.DELETE_DELTA_PREFIX)) {
+ AcidUtils.ParsedDelta delta = AcidUtils.parsedDelta(stat, fs);
Review comment:
It would be helpful to extract this logic to a separate method.
##########
File path: common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
##########
@@ -3058,6 +3058,11 @@ private static void populateLlapDaemonVarsSet(Set<String> llapDaemonVarsSetLocal
HIVE_COMPACTOR_CLEANER_RUN_INTERVAL("hive.compactor.cleaner.run.interval", "5000ms",
new TimeValidator(TimeUnit.MILLISECONDS), "Time between runs of the cleaner thread"),
+ HIVE_COMPACTOR_DELAYED_CLEANUP_ENABLED("hive.compactor.delayed.cleanup.enabled", false,
+ "When enabled, cleanup of obsolete files/dirs after compaction can be delayed. This delay \n" +
+ " can be configured by hive configuration hive.compactor.cleaner.retention.time.seconds"),
+ HIVE_COMPACTOR_CLEANER_RETENTION_TIME_SECONDS("hive.compactor.cleaner.retention.time.seconds", "300s",
Review comment:
It might be better to change the name to "HIVE_COMPACTOR_CLEANER_RETENTION_TIME", since the value would indicate whether it's in seconds or milliseconds.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org