Skip to content

Core: delete previous metadata locations in dropping table with purge enabled#3622

Merged
yyanyy merged 2 commits into
apache:masterfrom
yyanyy:delete-prev-metadata-log
Dec 1, 2021
Merged

Core: delete previous metadata locations in dropping table with purge enabled#3622
yyanyy merged 2 commits into
apache:masterfrom
yyanyy:delete-prev-metadata-log

Conversation

@yyanyy

@yyanyy yyanyy commented Nov 29, 2021

Copy link
Copy Markdown
Contributor

This change deletes the old metadata.json files in dropTableData which will be invoked by drop table with purge=true. Today only the latest metadata.json file will be deleted. Please see email thread https://lists.apache.org/thread/p59hn1dqoqs4r3z90tvlxx2wmv01g6fs for details.

I don't think we have any existing unit test around this, but I have tested the change locally and also added unit tests for the same method.

@github-actions github-actions Bot added the core label Nov 29, 2021

Tasks.foreach(Iterables.transform(metadata.previousFiles(), TableMetadata.MetadataLogEntry::file))
.noRetry().suppressFailureWhenFinished()
.onFailure((manifest, exc) -> LOG.warn("Delete failed for previous metadata file: {}", manifest, exc))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

variable name is not manifest, maybe metadataFile?

@jackye1995

Copy link
Copy Markdown
Contributor

Could you add a check in tests to verify the behavior?

@kbendick

Copy link
Copy Markdown
Contributor

Could you add a check in tests to verify the behavior?

Agreed. A test that actually lists the files afterwards would be really helpful.

@jackye1995 jackye1995 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good to me!

@kbendick kbendick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a great fix @yyanyy! Thank you!

@yyanyy

yyanyy commented Dec 1, 2021

Copy link
Copy Markdown
Contributor Author

Thank you @jackye1995 @kbendick for the fast review! I'll leave this PR open till tomorrow morning, if there are no further comments I'll merge it.

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.

3 participants