[bug][coordinator/kv] delete remote kv dir for table on coordinator server #297
Conversation
Vipamp
changed the title
|
@Vipamp Is this pr ready for review? |
Yes，i have finished it，i am sorry that i didn't tell you for review. |
| * | ||
| * </pre> | ||
| * | ||
| * @param remoteKvOrLogBaseDir - the remote kv snapshots or log segments root dir of specific |
There was a problem hiding this comment.
nit:
| * @param remoteKvOrLogBaseDir - the remote kv snapshots or log segments root dir of specific | |
| * @param remoteKvOrLogBaseDir - the remote kv snapshots or log segments root dir of tables |
| this.coordinatorEventProcessor = | ||
| new CoordinatorEventProcessor( | ||
| zkClient, | ||
| remoteStorageHandler, |
There was a problem hiding this comment.
nit: not to define local variable, just pass new RemoteStorageHandler(conf);
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.io.IOException; | ||
|
|
There was a problem hiding this comment.
Please add comments for this class..
|
|
||
| import java.io.IOException; | ||
|
|
||
| public class RemoteStorageHandler { |
There was a problem hiding this comment.
Rename it to RemoteStorageCleaner ? as now it's only for delete remote storage dir....
| this.remoteFileSystem = remoteKvDir.getFileSystem(); | ||
| } | ||
|
|
||
| public void createTableKvDir(TablePath tablePath, long tableId) throws IOException { |
There was a problem hiding this comment.
do not introudce this method since it's only for testing... introduce code used only in testing will make it hard to maintain.
|
|
||
| private void deleteDir(FsPath fsPath) { | ||
| try { | ||
| if (isDirExists(fsPath)) { |
There was a problem hiding this comment.
nit: just use remoteFileSystem.exists(fsPath)
| } | ||
| } | ||
|
|
||
| private FsPath tableKvDir(TablePath tablePath, long tableId) { |
There was a problem hiding this comment.
| private FsPath tableKvDir(TablePath tablePath, long tableId) { | |
| private FsPath tableKvRemoteDir(TablePath tablePath, long tableId) { |
| remoteFileSystem.mkdirs(tableKvDir(tablePath, tableId)); | ||
| } | ||
|
|
||
| public void deleteTable(TablePath tablePath, long tableId) { |
There was a problem hiding this comment.
| public void deleteTable(TablePath tablePath, long tableId) { | |
| public void deleteTableRemoteDir(TablePath tablePath, long tableId) { |
| import java.nio.file.Path; | ||
|
|
||
| import static org.assertj.core.api.AssertionsForClassTypes.assertThat; | ||
|
|
There was a problem hiding this comment.
Please add comments for this class... Otherwise, checkstyle will fail
| @@ -159,6 +172,9 @@ void testDeleteTable() throws Exception { | |||
| assertThat(zookeeperClient.getTableAssignment(tableId)).isEmpty(); | |||
| // the table will also be removed from coordinator context | |||
| assertThat(coordinatorContext.getAllReplicasForTable(tableId)).isEmpty(); | |||
|
|
|||
| // the table's remote kv root dir should be removed. | |||
| assertThat(remoteStorageHandler.isTableKvDirExists(DATA1_TABLE_PATH_PK, tableId)).isFalse(); | |||
There was a problem hiding this comment.
Also add IT to make sure drop table will also delete kv remote dir.
Maybe we can add in KvSnapshotITCase
Thinks for your comments，i will update those codes later. |
|
@luoyuxia Hi, I have updated those code, PTAL, thinks. |
| } | ||
|
|
||
| @Test | ||
| void testKvSnapshot() throws Exception { | ||
| void testKvSnapshotAndDeleteForTabletServer() throws Exception { |
There was a problem hiding this comment.
| void testKvSnapshotAndDeleteForTabletServer() throws Exception { | |
| void testKvSnapshotAndDelete() throws Exception { |
| completedSnapshotHandleStore = | ||
| new ZooKeeperCompletedSnapshotHandleStore( | ||
| FLUSS_CLUSTER_EXTENSION.getZooKeeperClient()); | ||
| CoordinatorServer coordinatorServer = FLUSS_CLUSTER_EXTENSION.getCoordinatorServer(); | ||
| Field coordinatorEventProcessorField = |
There was a problem hiding this comment.
It's hack to get CoordinatorEventProcessor by reflection..
| .isEqualTo(6)); | ||
| } | ||
| } | ||
| eventProcessor.process(new DropTableEvent(tableId, false)); |
There was a problem hiding this comment.
Can we just use CoordinatorService#dropTable to mock the deletion of table? Move the logic to test method testKvSnapshotAndDeleteForTabletServer so that
1: the test can be removed
2: we can remove the code in method testKvSnapshotAndDeleteForTabletServer
CompletableFuture<List<StopReplicaResultForBucket>> future =
new CompletableFuture<>();
server.getReplicaManager()
.stopReplicas(
INITIAL_COORDINATOR_EPOCH,
Collections.singletonList(
new StopReplicaData(
tableBucket, true, INITIAL_COORDINATOR_EPOCH, 1)),
future::complete);
and replace it with method CoordinatorService#dropTable directly to simpify the testing..
|
hi, @luoyuxia I have updated the codes, PTAL, thinks. |
Purpose
Linked issue: close #296
Tests
KvSnapshotITCase#testKvSnapshotAndDelete
API and Format
Documentation