Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Support the rollback and orphan file clean with changelog deco… #3144

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

Aitozi
Copy link
Contributor

@Aitozi Aitozi commented Apr 2, 2024

…uple

Purpose

The third step of #2899

Handle the orphan file cleaner with the changelog metadata

Tests

API and Format

Documentation

taggedSnapshot.schemaId(),
taggedSnapshot.baseManifestList(),
taggedSnapshot.deltaManifestList(),
null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to 'mute' the changelogManifestList here, if the changelog and snapshot already not there. Otherwise, the scanner may read on a non-exist manifest list. WDYT ? @JingsongLi

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you show the exception stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

It can be reproduce by disable this block and rerun testRollbackWithChangelog

Copy link
Contributor

Choose a reason for hiding this comment

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

I see testRollbackWithChangelog, but rollbacked table has no changelog, why you want to read changelog?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is a separate issue which is unrelated to changelog decouple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see testRollbackWithChangelog, but rollbacked table has no changelog, why you want to read changelog?

But there is no check on this now. After rollback, user can still specify a point to run the stream read, if the id is point to the rollback snapshot, then it will fail with the above exception.

In another thought, If we support decouple the changelog lifecycle, I have a table with 2 days changelog,

2024-04-01
2024-04-02

then I rollback the table to 2024-04-02 00:00, I can still stream read from 2024-04-01 to 2024-04-02 00:00. But we can not stream read before the 2024-04-01.

So, my main point is to prevent user read on a non-exist changelog data with an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discuss with @JingsongLi , it's a separate issue, we could solve it in another pr. So revert this change.

@Aitozi
Copy link
Contributor Author

Aitozi commented Apr 2, 2024

Is the failure CI related to the recent commit ?

@JingsongLi
Copy link
Contributor

Is the failure CI related to the recent commit ?

rebase latest master

@Aitozi Aitozi force-pushed the changelog-orphanfile branch from 5e5f33d to e8b996a Compare April 2, 2024 09:14
@Aitozi Aitozi force-pushed the changelog-orphanfile branch from 9ee6be6 to bff560e Compare April 3, 2024 10:49
@Aitozi Aitozi requested a review from JingsongLi April 3, 2024 12:07
Copy link
Contributor

@JingsongLi JingsongLi left a comment

Choose a reason for hiding this comment

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

+1

@JingsongLi JingsongLi merged commit 478f167 into apache:master Apr 7, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants