You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "lintingbin (via GitHub)" <gi...@apache.org> on 2024/09/10 10:35:46 UTC
[PR] Add the "clean-orphan-file.interval-time-minutes" parameter [amoro]
lintingbin opened a new pull request, #3193:
URL: https://github.com/apache/amoro/pull/3193
<!--
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://amoro.apache.org/how-to-contribute/
2. If the PR is related to an issue in https://github.com/apache/amoro/issues, add '[AMORO-XXXX]' in your PR title, e.g., '[AMORO-XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][AMORO-XXXX] Your PR title ...'.
-->
## Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about its use case.
2. If you fix a bug, you can clarify why it is a bug.
3. Use Fix/Resolve/Close #{ISSUE_NUMBER} to link this PR to its related issue
-->
Close #3192.
## Brief change log
<!--
Clearly describe the changes made in modules, classes, methods, etc.
-->
- Add a parameter to replace the INTERVAL constant, which was previously fixed at 24 hours in the OrphanFilesCleaningExecutor class.
## How was this patch tested?
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x] Run test locally before making a pull request
## Documentation
- Does this pull request introduce a new feature? yes
- If yes, how is the feature documented? docs
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the "clean-orphan-file.interval-time-minutes" parameter [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on PR #3193:
URL: https://github.com/apache/amoro/pull/3193#issuecomment-2342789957
> Why is this necessary? The "min existing time" ensures that useful files are not mistakenly deleted, so setting it to two to three days is usually sufficient. However, cleaning orphan files is a performance-intensive task, so it's generally appropriate to configure the cleanup to occur once a week or once a month.
I agree with @lintingbin, there is no connection between the values of these two configurations.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the "clean-orphan-file.interval-time-minutes" parameter [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on PR #3193:
URL: https://github.com/apache/amoro/pull/3193#issuecomment-2342784371
@lintingbin Thanks for the contribution.
The scheduling interval for the current expiration task is usually set as an AMS configuration like `data-expiration.interval`, which applies to all tables. I think this meets the requirements, as we generally do not adjust different execution interval for different tables. What do you think?
BTW, we are improving the configuration value for time duration with unit support like 3d, we can do it for the new configuration.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3192] Add the "clean-orphan-file.interval-time-minutes" parameter [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong merged PR #3193:
URL: https://github.com/apache/amoro/pull/3193
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3192] Add the "clean-orphan-file.interval-time-minutes" parameter [amoro]
Posted by "lintingbin (via GitHub)" <gi...@apache.org>.
lintingbin commented on PR #3193:
URL: https://github.com/apache/amoro/pull/3193#issuecomment-2343168502
> @lintingbin Thanks for the contribution.
>
> The scheduling interval for the current expiration task is usually set as an AMS configuration like `data-expiration.interval`, which applies to all tables. I think this meets the requirements, as we generally do not adjust different execution interval for different tables. What do you think?
>
> BTW, we are improving the configuration value for time duration with unit support like 3d, we can do it for the new configuration.
The modifications have been completed according to the suggestions.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add the "clean-orphan-file.interval-time-minutes" parameter [amoro]
Posted by "lintingbin (via GitHub)" <gi...@apache.org>.
lintingbin commented on PR #3193:
URL: https://github.com/apache/amoro/pull/3193#issuecomment-2342640250
> Thanks for your work, but I think the default check interval should be less than the default min existing time.
Why is this necessary? The "min existing time" ensures that useful files are not mistakenly deleted, so setting it to two to three days is usually sufficient. However, cleaning orphan files is a performance-intensive task, so it's generally appropriate to configure the cleanup to occur once a week or once a month.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3192] Add the "clean-orphan-file.interval-time-minutes" parameter [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on code in PR #3193:
URL: https://github.com/apache/amoro/pull/3193#discussion_r1753864098
##########
docs/user-guides/configurations.md:
##########
@@ -72,6 +72,7 @@ Data-cleaning configurations are applicable to both Iceberg Format and Mixed str
| snapshot.keep.duration | 720min(12 hours) | Table-Expiration keeps the latest snapshots within a specified duration |
| snapshot.keep.min-count | 1 | Minimum number of snapshots retained for table expiration |
| clean-orphan-file.enabled | false | Enables periodically clean orphan files |
+| clean-orphan-file.interval | 1d | The interval for Cleaning orphan files within a specified time |
Review Comment:
This change may need to be rollback
And you may want to add this new configuration example into `dist/src/main/amoro-bin/conf/config.yaml`
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org