Service: Cleanup Iceberg Manifest list on Purge #1039
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
singhpk234
requested review from
MonkeyCanCode,
RussellSpitzer,
adutra,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
Contributor
|
cc @XJDKC |
flyrain
approved these changes
Feb 21, 2025
Contributor
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fix, @singhpk234 !
Contributor
|
Looks like a conflict happen due to a new commit. I'm happy to merge it after it's fixed. |
singhpk234
force-pushed
the
fix/purge-cleanup
branch
from
9cea7cb to
125e100
Compare
Contributor
Author
|
Ack, just rebased and re-ran the UT ! |
flyrain
approved these changes
Feb 24, 2025
eric-maynard
added a commit
to eric-maynard/polaris
that referenced
this pull request
Mar 13, 2025
* Publish proposals and roadmap on the website (apache#1043) * Allow Polaris CLI to set property values that contain '=' (apache#1003) Currently the CLI parsing is too pessimistic and throws an error if it detects '=' in the parsed value of a property. However, the split 1 semantics are already standard behavior for most parsers where the right-hand-side is allowed to contain '='. This commonly comes up if the value is itself a comma-separated kv list or if it the value is a base64-encoded string. * main: Update postgres Docker tag to v17.4 (apache#1049) * main: Update dependency org.junit:junit-bom to v5.12.0 (apache#1045) * Revert "Remove CallContext and its ThreadLocal usage" (apache#1000) * revert b84f462 * resolve conflicts * autolint * autolint * ...not possible to automatically add a synthetic no-args constructor to an unproxyable bean class. * fix * fixed non-quarkus build issues * fixed non-quarkus build issues * autolint * more di issues * add more producers * add another producer * autolint * change callcontext usage in FileIOFactoryTest * stablize tests * autolint * fix a test * refactor TableCleanupTaskHandlerTest * another fix, still npe * autolint * only task cleanup is problematic * autolint * Stabilize more tests * autolint * Avoid propagating old callcontexts across TestServices instances * testservices update * autolint * Revert "autolint" This reverts commit 66a7f22. * Revert "testservices update" This reverts commit 897a5a1. * changes to taskexecutorimpl * autolint --------- Co-authored-by: Dennis Huo <[email protected]> * Enable console color when running Polaris with Gradle (apache#977) * CLI: add subcommand access for principals (apache#1019) * Access subcommand access for principals * Access subcommand access for principals * Fix getting start spark (apache#1054) * Use correct link format for adoc and update menu title for proposals (apache#1051) * Add a debug log with the full exception (apache#1023) * clean up * spotless * clean up * refactor message * no CVEM * change log levels * revert polaris-java dep change * add testImplementation * uppercase * Remove all tightly coupled EntityCache dependencies in the main persistence stack (apache#1055) Remove the EntityCacheEntry wrapper since the timestamp fields were never used anyways; instead the underlying Caffeine cache transparently handles access times, and the types of entries we cache are simply the existing ResolvedPolarisEntity. Remove interactions of business logic with explicit "cache entries", instead operating on ResolvedPolarisEntity. Fix the equals()/hashCode() behavior of PolarisEntity to be compatible with PolarisBaseEntity as intended. Improve code comments to explain the (current) relationship between PolarisEntity and PolarisBaseEntity, and clarify the behaviors in Resolver.java. Fully remove the PolarisRemoteCache interface and its methods. Add different methods that aren't cache-specific instead. * Remove PolarisMetaStoreSession from FileIOFactory/FileIOUtil in favor of CallContext (apache#1057) This appeared to be some leaky divergence that occurred after CallContext had been removed, but PolarisMetaStoreSession really is only a low-level implementation detail that should never be handled by BasePolarisCatalog/FileIOFactory. This plumbs CallContext explicitly into the FileIOFactory and FileIOUtil methods and thus removes a large source of CallContext.getCurrentContext calls; now the threadlocal doesn't have to be set at all in BasePolarisCatalogTest. * Cleanup Iceberg Manifest list files on Purge (apache#1039) * Fix small quickstart typo (apache#1061) * Enable rat check on md files and fix the existing ones (apache#1050) * main: Update dependency boto3 to v1.37.0 (apache#1052) * main: Update dependency software.amazon.awssdk:bom to v2.30.27 (apache#1053) * Integration tests: improve support for temporary folders (apache#987) 3 integration tests currently require a temporary folder for various purposes. The folders are generally hard-coded to some local filesystem location, which makes those tests impossible to execute against a remote Polaris instance. This PR aims at fixing this situation by leveraging a new optional environment variable that points to a location that is accessible by both the server and the client running the tests. This would typically be an S3 bucket, for example, but could also be a shared volume mounted on both machines at the same mount point. If this environment variable is not present, then local execution is assumed and a temporary directory managed by JUnit is used instead. * build-logic: exclude generated code from errorprone (apache#1035) * build-logic: let javadoc depend on jandex (apache#1034) * Add Prashant in the collaborators list (apache#1064) * main: Update dependency org.slf4j:slf4j-api to v2.0.17 (apache#1065) --------- Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Dennis Huo <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Dennis Huo <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: MonkeyCanCode <[email protected]> Co-authored-by: Andrew Guterman <[email protected]> Co-authored-by: Prashant Singh <[email protected]> Co-authored-by: Robert Stupp <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
About the change
Presently cleaning up of ManifestList is missing when we call the table drop with purge. This change aims to fix this issue by adding manifest list also to the metadata cleanup part of the task.
Solve bug reported via: #967
Testing done
Updated existing test cases