Spark: Remove deletefiles when expiring snapshots. #2518
Conversation
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
I suppose some code will be cleaned out after #2510 is merged, so please mark this as draft first.
| } | ||
|
|
||
| public ExpireSnapshotsActionResult(Long dataFilesDeleted, Long manifestFilesDeleted, Long manifestListsDeleted) { | ||
| public ExpireSnapshotsActionResult(Long dataFilesDeleted, Long deleteFilesDeleted, |
There was a problem hiding this comment.
for public class, should keep existing constructor and add new ones.
| private String path = null; | ||
| private Long length = null; | ||
| private Integer partitionSpecId = null; | ||
| private Integer content = null; |
There was a problem hiding this comment.
why not just use ManifestContent instead of Integer?
| JavaSparkContext context = new JavaSparkContext(spark.sparkContext()); | ||
| Broadcast<FileIO> ioBroadcast = context.broadcast(SparkUtil.serializableFileIO(table)); | ||
| return loadAllManifestFileBean(table).filter((FilterFunction<ManifestFileBean>) manifest -> | ||
| manifest.content() == ManifestContent.DELETES) |
There was a problem hiding this comment.
should compare using MnaifestContent.DELETES.equals(...)
| JavaSparkContext context = new JavaSparkContext(spark.sparkContext()); | ||
| Broadcast<FileIO> ioBroadcast = context.broadcast(SparkUtil.serializableFileIO(table)); | ||
| return loadAllManifestFileBean(table).filter((FilterFunction<ManifestFileBean>) manifest -> | ||
| manifest.content() == ManifestContent.DATA) |
There was a problem hiding this comment.
should compare using MnaifestContent.DATA.equals(...)
There was a problem hiding this comment.
I guess == is also correct for enum
| int totalJobsRun = jobsAfter - jobsBefore; | ||
|
|
||
| checkExpirationResults(1L, 1L, 2L, results); | ||
| checkExpirationResults(1L, 0L, 1L, 2L, results); |
There was a problem hiding this comment.
I don't see a test that actually tests delete file count > 0, could you add one?
There was a problem hiding this comment.
Thanks, I will add the unit tests.
| protected Dataset<Row> buildValidDeleteFileDF(Table table) { | ||
| JavaSparkContext context = new JavaSparkContext(spark.sparkContext()); | ||
| Broadcast<FileIO> ioBroadcast = context.broadcast(SparkUtil.serializableFileIO(table)); | ||
| return loadAllManifestFileBean(table).filter((FilterFunction<ManifestFileBean>) manifest -> |
There was a problem hiding this comment.
is it worthwhile to separate the loadAllManifestFileBean into 2 passes instead of one?
|
@coolderli are you still on this? pleasure to pick up where you left and push this forward. |
|
Oh I am sorry, I did not notice this pr (should have checked), I made a similar pr here: #4629, if you want to look at it. Although I do not change the manifest table as is done here, which may still be valuable as a separate pr. By the way, the delete files are actually deleted, (I added a test in #4141) , just the return counts are wrong. |
thanks for sharing, this one should be closed then. |
|
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 [email protected] 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. |
5 participants
When expiring snapshots, the deletefiles should be deleted.