Skip to content

Conversation

@amogh-jahagirdar
Copy link
Contributor

Follow up PR to #5669.

1.) Parallelizing determining the reachable manifests given a set of snapshots

2.) Cleaning up issues in log messages #5669 (comment) and #5669 (comment)

@github-actions github-actions bot added the core label Oct 14, 2022
@amogh-jahagirdar amogh-jahagirdar changed the title Core: Parallelize the determining of reachable manifests during snapshot expiration file cleanp Core: Parallelize the determining of reachable manifests during file cleanup Oct 14, 2022
@amogh-jahagirdar amogh-jahagirdar force-pushed the reachability-cleanup branch 5 times, most recently from ed9e051 to b061989 Compare October 15, 2022 04:04
@amogh-jahagirdar amogh-jahagirdar requested review from kbendick and rdblue and removed request for kbendick and rdblue October 16, 2022 16:57
@amogh-jahagirdar amogh-jahagirdar force-pushed the reachability-cleanup branch 2 times, most recently from 10eca9a to 24a456a Compare October 16, 2022 17:11
@amogh-jahagirdar amogh-jahagirdar force-pushed the reachability-cleanup branch 2 times, most recently from 06cc886 to a205384 Compare October 17, 2022 01:55
Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

return;
}

currentManifestCallback.accept(manifestFile.copy());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor, but it seems weird to copy here rather than in the callback. If the callback were a noop, we'd be doing work for nothing. Since we know it needs to be copied, it seems fine though.

@rdblue rdblue merged commit 3ae58a1 into apache:master Oct 19, 2022
@rdblue
Copy link
Contributor

rdblue commented Oct 19, 2022

Thanks, @amogh-jahagirdar! Looks great.

@amogh-jahagirdar
Copy link
Contributor Author

Thanks for the reviews @rdblue @kbendick @jackye1995 !

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants