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

feat: refactor manifest to get snapshot in memory #825

Merged

Conversation

Rachelint
Copy link
Contributor

Which issue does this PR close?

Closes #
Part of #799

Rationale for this change

Make snapshot able to get snapshot from table data.

What changes are included in this PR?

Make snapshot able to get snapshot from table data.

Are there any user-facing changes?

None.

How does this change test

Test by ut.

@Rachelint Rachelint force-pushed the refactor-manifest-to-get-snapshot-in-memory branch 2 times, most recently from 1c1af66 to 058e57e Compare April 18, 2023 05:51
Copy link
Member

@ShiKaiWi ShiKaiWi left a comment

Choose a reason for hiding this comment

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

This solution sounds good. However, there may be one potential problem in it. Here is an example for it:

  1. The manifest wal is updated;
  2. Snapshot is triggered;
  3. Table metadata in the memory is used to form the latest snapshot;

In the step 3, the table metadata (snapshot) may be different from the logs from manifest wal.

analytic_engine/src/instance/open.rs Outdated Show resolved Hide resolved
analytic_engine/src/setup.rs Outdated Show resolved Hide resolved
analytic_engine/src/manifest/details.rs Outdated Show resolved Hide resolved
analytic_engine/src/manifest/details.rs Outdated Show resolved Hide resolved
analytic_engine/src/manifest/details.rs Outdated Show resolved Hide resolved
@Rachelint Rachelint force-pushed the refactor-manifest-to-get-snapshot-in-memory branch from 5fd66b0 to 3e49670 Compare April 19, 2023 02:37
@Rachelint Rachelint force-pushed the refactor-manifest-to-get-snapshot-in-memory branch from 3e49670 to 8ff80ed Compare April 19, 2023 02:38
@Rachelint
Copy link
Contributor Author

Rachelint commented Apr 19, 2023

This solution sounds good. However, there may be one potential problem in it. Here is an example for it:

1. The manifest wal is updated;

2. Snapshot is triggered;

3. Table metadata in the memory is used to form the latest snapshot;

In the step 3, the table metadata (snapshot) may be different from the logs from manifest wal.

In fact, this situation will not be occurred.
Snapshot works always done before writing the new comming logs to log store.

@ShiKaiWi
Copy link
Member

This solution sounds good. However, there may be one potential problem in it. Here is an example for it:

1. The manifest wal is updated;

2. Snapshot is triggered;

3. Table metadata in the memory is used to form the latest snapshot;

In the step 3, the table metadata (snapshot) may be different from the logs from manifest wal.

In fact, this situation will not be occurred. Snapshot works always done before writing the new comming logs to log store.

After some thoughts, I'm still worried about that the table metadata in the memory is not controlled by the manifest, that is to say, the order of updating manifest before the table metadata in memory is implicit. Maybe we can extend the TableSnapshotProvider to TabeSnapshotManager which is not only able to provide the latest snapshot but also receive metadata updates.

@Rachelint
Copy link
Contributor Author

This solution sounds good. However, there may be one potential problem in it. Here is an example for it:

1. The manifest wal is updated;

2. Snapshot is triggered;

3. Table metadata in the memory is used to form the latest snapshot;

In the step 3, the table metadata (snapshot) may be different from the logs from manifest wal.

In fact, this situation will not be occurred. Snapshot works always done before writing the new comming logs to log store.

After some thoughts, I'm still worried about that the table metadata in the memory is not controlled by the manifest, that is to say, the order of updating manifest before the table metadata in memory is implicit. Maybe we can extend the TableSnapshotProvider to TabeSnapshotManager which is not only able to provide the latest snapshot but also receive metadata updates.

I will sovle it in next pr.

Copy link
Member

@ShiKaiWi ShiKaiWi left a comment

Choose a reason for hiding this comment

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

LGTM

@ShiKaiWi ShiKaiWi added this pull request to the merge queue Apr 19, 2023
Merged via the queue into apache:main with commit ede19fa Apr 19, 2023
chunshao90 pushed a commit to chunshao90/ceresdb that referenced this pull request May 15, 2023
* support return snapshot in `TableVersion`.

* introdce `TableSnapshotProvider` to get manifest snapshot data from memory.

* refactor `Snapshotter` to support from memory or storage.

* correct the order between do snapshot and write wal.

* fix test in manifest.

* let table snapshot provider return None when drop table.

* remove `origin_logs_num` from `Snapshot`.

* fix compile and clippy.

* split `Snapshotter` to `Snapshotter` and `SnapshotRecoverer`, remove the now unused `SnapshotBuilder`.

* move `TableSnapshotProviderImpl` to space module.
@Rachelint Rachelint deleted the refactor-manifest-to-get-snapshot-in-memory branch May 27, 2023 12:18
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