File size in bytes tracking with deleted files in expire snapshots #10036
Conversation
There was a problem hiding this comment.
Definitely the right path, if you wanted to separate out bulk from single deletes I think that's also valid. Probably simpler to get the non-bulk path working first.
Note we need tests here to make sure everything is working properly but I think you have the right approach here.
| this.sizeInBytes = sizeInBytes; | ||
| } | ||
| // Getter and setter for sizeInBytes | ||
| public long getSizeInBytes() { |
There was a problem hiding this comment.
just FYI that the Iceberg project doesn't use get/set prefixes
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
sasankpagolu
force-pushed
the
exp-snapshots-file-bytes
branch
from
8c038a4 to
2762f31
Compare
github-actions
bot
removed
MR
ORC
INFRA
AWS
NESSIE
ALIYUN
GCP
DELL
SNOWFLAKE
OPENAPI
AZURE
KAFKACONNECT
stale
labels
There was a problem hiding this comment.
thank you @sasankpagolu for your PR
- looks like there's some CI failure that you might want to take a look?
- I also left some comment to see if we can avoid introduce exclusive new class, also I am wondering if this is only meant to be used in hadoop fileIO or also applicable to other fileIO as well?
| */ | ||
| package org.apache.iceberg.spark.actions; | ||
|
|
||
| public class RichFileInfo extends FileInfo { |
There was a problem hiding this comment.
Instead of defining this, I am wondering if you can reuse the existing iceberg.io.FileInfo , with location, size and created timestamp.
Alternatively, I do notice existing iceberg.spark.FileInfo only have path and type tho. Maybe also worth considering add one optional size ?
| */ | ||
| package org.apache.iceberg.io; | ||
|
|
||
| public class FileInfoSummary { |
There was a problem hiding this comment.
I believe this can be replaced with iceberg.io.FileInfo
| public void deleteFilesWithSummary(Iterable<FileInfoSummary> filesToDelete) | ||
| throws BulkDeletionFailureException { | ||
| AtomicInteger failureCount = new AtomicInteger(0); | ||
| AtomicLong failureFileSize = new AtomicLong(0); | ||
| Tasks.foreach(filesToDelete) | ||
| .executeWith(executorService()) | ||
| .retry(DELETE_RETRY_ATTEMPTS) | ||
| .stopRetryOn(FileNotFoundException.class) | ||
| .suppressFailureWhenFinished() | ||
| .onFailure( | ||
| (f, e) -> { | ||
| LOG.error("Failure during bulk delete on file: {} ", f.location(), e); | ||
| failureCount.incrementAndGet(); | ||
| failureFileSize.addAndGet(f.size()); | ||
| }) | ||
| .run(fileInfo -> deleteFile(fileInfo.location())); | ||
|
|
||
| if (failureCount.get() != 0) { | ||
| throw new BulkDeletionFailureException(failureCount.get(), failureFileSize.get()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we really need this new method and override only in HadoopFileIO? I think we can extract the path from FileInfoSummary and use for deletion. The failureFileSize can be summed over failure path
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
No description provided.