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

[server/kv]fix bug for deleting remote kv dir #245

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Vipamp
Copy link

@Vipamp Vipamp commented Dec 20, 2024

Purpose

Linked issue: close #121
this pr is aims to solve the bug mentioned in #121

Tests

API and Format

Documentation

@Vipamp Vipamp changed the title [#121]fix bug for delete remote kv dir [server/kv]fix bug for deleting remote kv dir Dec 20, 2024
@luoyuxia luoyuxia added this to the v0.6 milestone Dec 23, 2024
@luoyuxia luoyuxia self-requested a review December 23, 2024 01:43
Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

Please add unit tests or IT cases to verify the fixing. An ideal place to add IT cases is KvSnapshotITCase.

KvTablet dropKvTablet =
inLock(tabletCreationOrDeletionLock, () -> currentKvs.remove(tableBucket));

if (dropKvTablet != null) {
TablePath tablePath = dropKvTablet.getTablePath();
try {
dropKvTablet.drop();
if (deleteRemote) {
dropRemoteKvSnapshot(physicalTablePath, tableBucket);
Copy link
Member

Choose a reason for hiding this comment

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

This removal is very similar to remote log removal. We can move this invoking KvManager.dropRemoteKvSnapshot into ReplicaManager#stopReplica after remoteLogManager.stopReplica. Ideally, we should delete the remote files (log&kv) asynchronously to not block the worker thread. We can refactor them in the near future.

Copy link
Author

Choose a reason for hiding this comment

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

Yes,i have notice that,I find RemoteLogManager to manager remote files of LogTable without RemoteKvManager to manager snapshot files of KvTable,is there any plan to unify those for managering remote files? I will continue to pay attention about it and look forward to participating it.

@Vipamp
Copy link
Author

Vipamp commented Dec 23, 2024

Please add unit tests or IT cases to verify the fixing. An ideal place to add IT cases is KvSnapshotITCase.

Thank you for your guidance,I will add it.

@Vipamp
Copy link
Author

Vipamp commented Dec 27, 2024

@wuchong PTAL,thinks

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.

[Bug] Dropping primary key table doesn't delete kv snapshot
3 participants