You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "xichen01 (via GitHub)" <gi...@apache.org> on 2023/06/16 07:28:43 UTC
[GitHub] [ozone] xichen01 opened a new pull request, #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
xichen01 opened a new pull request, #4913:
URL: https://github.com/apache/ozone/pull/4913
## What changes were proposed in this pull request?
Some performance issues are mentioned here. https://issues.apache.org/jira/browse/HDDS-8865 so in this PR:
We Make DN `DeleteBlocksCommandHandler` wait for the lock to timeout. This way, no other transaction will be blocked because the lock cannot be acquired, and for locks that time out, we can retry once on the DN if can not acquired the lock in specified time, transaction will failed and the SCM can resent it after.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8869
## How was this patch tested?
unit test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #4913:
URL: https://github.com/apache/ozone/pull/4913
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xichen01 commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1333345614
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -188,6 +188,24 @@ public class DatanodeConfiguration {
private long recoveringContainerScrubInterval =
Duration.ofMinutes(10).toMillis();
+ /**
+ * Timeout for the thread used to process the delete block command
+ * to wait for the container lock.
+ * It takes about 200ms to open a RocksDB with HDD media.
+ * Set the default value to 100ms, so that after one times retry
+ * after waiting timeout, the hold time spent waiting for the lock
+ * is not greater than the time spent operating RocksDB
+ */
+ @Config(key = "block.delete.command.handle.lock.timeout",
Review Comment:
This comment may have caused some misunderstanding, I'll modify it.
The retry does not happen immediately, it waits until all `DeleteBlockTransactions` have been processed, so the retry may happen after a few seconds or tens of seconds.
Slow RocksDB open (Schema V2 only), rebalancer, `BlockDeletingService`, etc, may cause here cannot get the lock, retry to increase the chance of success.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -346,10 +370,75 @@ private void processCmd(DeleteCmdInfo cmd) {
}
}
+ @VisibleForTesting
+ public List<DeleteBlockTransactionResult> executeCmdWithRetry(
+ List<DeletedBlocksTransaction> transactions) {
+ List<DeleteBlockTransactionResult> results =
+ new ArrayList<>(transactions.size());
+ Map<Long, DeletedBlocksTransaction> idToTransaction =
+ new HashMap<>(transactions.size());
+ transactions.forEach(tx -> idToTransaction.put(tx.getTxID(), tx));
+ List<DeletedBlocksTransaction> retryTransaction = new ArrayList<>();
+
+ List<Future<DeleteBlockTransactionExecutionResult>> futures =
+ submitTasks(transactions);
+ // Wait for tasks to finish
+ handleTasksResults(futures, result -> {
+ if (result.isLockAcquisitionFailed()) {
+ retryTransaction.add(idToTransaction.get(result.getResult().getTxID()));
+ } else {
+ results.add(result.getResult());
+ }
+ });
+
+ idToTransaction.clear();
+ // Wait for all tasks to complete before retrying, usually it takes
+ // some time for all tasks to complete, then the retry may be successful.
+ // We will only retry once
+ if (!retryTransaction.isEmpty()) {
+ futures = submitTasks(retryTransaction);
+ handleTasksResults(futures, result -> {
+ if (result.isLockAcquisitionFailed()) {
+ blockDeleteMetrics.incrTotalLockTimeoutTransactionCount();
+ }
+ results.add(result.getResult());
+ });
+ }
+ return results;
+ }
+
+ @VisibleForTesting
+ public List<Future<DeleteBlockTransactionExecutionResult>> submitTasks(
+ List<DeletedBlocksTransaction> deletedBlocksTransactions) {
+ List<Future<DeleteBlockTransactionExecutionResult>> futures =
+ new ArrayList<>(deletedBlocksTransactions.size());
+
+ for (DeletedBlocksTransaction tx : deletedBlocksTransactions) {
+ Future<DeleteBlockTransactionExecutionResult> future =
+ executor.submit(new ProcessTransactionTask(tx));
+ futures.add(future);
+ }
+ return futures;
+ }
+
+ public void handleTasksResults(
+ List<Future<DeleteBlockTransactionExecutionResult>> futures,
+ Consumer<DeleteBlockTransactionExecutionResult> handler) {
+ futures.forEach(f -> {
+ try {
+ DeleteBlockTransactionExecutionResult result = f.get();
+ handler.accept(result);
+ } catch (InterruptedException | ExecutionException e) {
+ LOG.error("task failed.", e);
+ Thread.currentThread().interrupt();
+ }
+ });
+ }
+
private void markBlocksForDeletionSchemaV3(
- KeyValueContainerData containerData, DeletedBlocksTransaction delTX,
- int newDeletionBlocks, long txnID)
+ KeyValueContainerData containerData, DeletedBlocksTransaction delTX)
throws IOException {
+ int newDeletionBlocks = 0;
Review Comment:
Already modified.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -232,40 +267,44 @@ public DeleteBlockTransactionResult call() {
ContainerType containerType = cont.getContainerType();
switch (containerType) {
case KeyValueContainer:
+ KeyValueContainer keyValueContainer = (KeyValueContainer)cont;
KeyValueContainerData containerData = (KeyValueContainerData)
cont.getContainerData();
- cont.writeLock();
- try {
- if (containerData.hasSchema(SCHEMA_V1)) {
- markBlocksForDeletionSchemaV1(containerData, tx);
- } else if (containerData.hasSchema(SCHEMA_V2)) {
- markBlocksForDeletionSchemaV2(containerData, tx,
- newDeletionBlocks, tx.getTxID());
- } else if (containerData.hasSchema(SCHEMA_V3)) {
- markBlocksForDeletionSchemaV3(containerData, tx,
- newDeletionBlocks, tx.getTxID());
- } else {
- throw new UnsupportedOperationException(
- "Only schema version 1,2,3 are supported.");
+ if (keyValueContainer.
+ writeLockTryLock(tryLockTimeoutMs, TimeUnit.MILLISECONDS)) {
+ try {
+ String schemaVersion = containerData
+ .getSupportedSchemaVersionOrDefault();
+ if (getSchemaHandlers().containsKey(schemaVersion)) {
+ schemaHandlers.get(schemaVersion).handle(containerData, tx);
+ } else {
+ throw new UnsupportedOperationException(
+ "Only schema version 1,2,3 are supported.");
+ }
+ } finally {
+ cont.writeUnlock();
}
- } finally {
- cont.writeUnlock();
+ txResultBuilder.setContainerID(containerId)
+ .setSuccess(true);
+ } else {
+ lockAcquisitionFailed = true;
+ txResultBuilder.setContainerID(containerId)
+ .setSuccess(false);
}
- txResultBuilder.setContainerID(containerId)
- .setSuccess(true);
break;
default:
LOG.error(
"Delete Blocks Command Handler is not implemented for " +
"containerType {}", containerType);
}
- } catch (IOException e) {
+ } catch (IOException | InterruptedException e) {
Review Comment:
Done.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/BlockDeletingServiceMetrics.java:
##########
@@ -74,6 +74,10 @@ public final class BlockDeletingServiceMetrics {
@Metric(about = "The total number of Container chosen to be deleted.")
private MutableGaugeLong totalContainerChosenCount;
+ @Metric(about = "The total number of transactions which was failed " +
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xichen01 commented on pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4913:
URL: https://github.com/apache/ozone/pull/4913#issuecomment-1634439255
@adoroszlai PTAL, Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1333820411
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -188,6 +188,24 @@ public class DatanodeConfiguration {
private long recoveringContainerScrubInterval =
Duration.ofMinutes(10).toMillis();
+ /**
+ * Timeout for the thread used to process the delete block command
+ * to wait for the container lock.
+ * It takes about 200ms to open a RocksDB with HDD media.
+ * Set the default value to 100ms, so that after one times retry
+ * after waiting timeout, the hold time spent waiting for the lock
+ * is not greater than the time spent operating RocksDB
+ */
+ @Config(key = "block.delete.command.handle.lock.timeout",
Review Comment:
@xichen01 , thanks for the further info.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xichen01 commented on pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4913:
URL: https://github.com/apache/ozone/pull/4913#issuecomment-1622920495
@sumitagrawl PTAL. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1331078837
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -188,6 +188,24 @@ public class DatanodeConfiguration {
private long recoveringContainerScrubInterval =
Duration.ofMinutes(10).toMillis();
+ /**
+ * Timeout for the thread used to process the delete block command
+ * to wait for the container lock.
+ * It takes about 200ms to open a RocksDB with HDD media.
+ * Set the default value to 100ms, so that after one times retry
+ * after waiting timeout, the hold time spent waiting for the lock
+ * is not greater than the time spent operating RocksDB
+ */
+ @Config(key = "block.delete.command.handle.lock.timeout",
Review Comment:
@xichen01 , is the slow RocksDB open a major cause behind this patch? I have one question, it takes 200ms to open a rocksdb, you set this timeout to 100ms, and retry once in the block deletion logic, why not set the default value to 200ms, with a simpler handler logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1331075094
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -346,10 +370,75 @@ private void processCmd(DeleteCmdInfo cmd) {
}
}
+ @VisibleForTesting
+ public List<DeleteBlockTransactionResult> executeCmdWithRetry(
+ List<DeletedBlocksTransaction> transactions) {
+ List<DeleteBlockTransactionResult> results =
+ new ArrayList<>(transactions.size());
+ Map<Long, DeletedBlocksTransaction> idToTransaction =
+ new HashMap<>(transactions.size());
+ transactions.forEach(tx -> idToTransaction.put(tx.getTxID(), tx));
+ List<DeletedBlocksTransaction> retryTransaction = new ArrayList<>();
+
+ List<Future<DeleteBlockTransactionExecutionResult>> futures =
+ submitTasks(transactions);
+ // Wait for tasks to finish
+ handleTasksResults(futures, result -> {
+ if (result.isLockAcquisitionFailed()) {
+ retryTransaction.add(idToTransaction.get(result.getResult().getTxID()));
+ } else {
+ results.add(result.getResult());
+ }
+ });
+
+ idToTransaction.clear();
+ // Wait for all tasks to complete before retrying, usually it takes
+ // some time for all tasks to complete, then the retry may be successful.
+ // We will only retry once
+ if (!retryTransaction.isEmpty()) {
+ futures = submitTasks(retryTransaction);
+ handleTasksResults(futures, result -> {
+ if (result.isLockAcquisitionFailed()) {
+ blockDeleteMetrics.incrTotalLockTimeoutTransactionCount();
+ }
+ results.add(result.getResult());
+ });
+ }
+ return results;
+ }
+
+ @VisibleForTesting
+ public List<Future<DeleteBlockTransactionExecutionResult>> submitTasks(
+ List<DeletedBlocksTransaction> deletedBlocksTransactions) {
+ List<Future<DeleteBlockTransactionExecutionResult>> futures =
+ new ArrayList<>(deletedBlocksTransactions.size());
+
+ for (DeletedBlocksTransaction tx : deletedBlocksTransactions) {
+ Future<DeleteBlockTransactionExecutionResult> future =
+ executor.submit(new ProcessTransactionTask(tx));
+ futures.add(future);
+ }
+ return futures;
+ }
+
+ public void handleTasksResults(
+ List<Future<DeleteBlockTransactionExecutionResult>> futures,
+ Consumer<DeleteBlockTransactionExecutionResult> handler) {
+ futures.forEach(f -> {
+ try {
+ DeleteBlockTransactionExecutionResult result = f.get();
+ handler.accept(result);
+ } catch (InterruptedException | ExecutionException e) {
+ LOG.error("task failed.", e);
+ Thread.currentThread().interrupt();
+ }
+ });
+ }
+
private void markBlocksForDeletionSchemaV3(
- KeyValueContainerData containerData, DeletedBlocksTransaction delTX,
- int newDeletionBlocks, long txnID)
+ KeyValueContainerData containerData, DeletedBlocksTransaction delTX)
throws IOException {
+ int newDeletionBlocks = 0;
Review Comment:
Could you do some refactor on markBlocksForDeletionTransaction too? This newDeletionBlocks is always 0. It's better to be declared in markBlocksForDeletionTransaction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xichen01 commented on pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4913:
URL: https://github.com/apache/ozone/pull/4913#issuecomment-1611877801
@sumitagrawl PTAL. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xichen01 commented on pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #4913:
URL: https://github.com/apache/ozone/pull/4913#issuecomment-1604606336
@adoroszlai PTAL. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1333822215
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -569,4 +661,25 @@ void apply(Table<?, DeletedBlocksTransaction> deleteTxnsTable,
BatchOperation batch, long txnID, DeletedBlocksTransaction delTX)
throws IOException;
}
+
+ /**
+ * The SchemaHandler interface provides a contract for handling
+ * KeyValueContainerData and DeletedBlocksTransaction based
+ * on different schema versions.
+ */
+ public interface SchemaHandler {
+ void handle(KeyValueContainerData containerData,
+ DeletedBlocksTransaction tx) throws IOException;
Review Comment:
@xichen01 , please correct the indent here. It's 4 blank space indent for a wrapped line. Same for TestSchemaHandler#handle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1245129895
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java:
##########
@@ -0,0 +1,288 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.container.common.statemachine.commandhandler;
+
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.DeletedBlocksTransaction;
+import org.apache.hadoop.ozone.container.ContainerTestHelper;
+import org.apache.hadoop.ozone.container.common.helpers.BlockDeletingServiceMetrics;
+import org.apache.hadoop.ozone.container.common.impl.ContainerLayoutVersion;
+import org.apache.hadoop.ozone.container.common.impl.ContainerSet;
+import org.apache.hadoop.ozone.container.common.interfaces.Container;
+import org.apache.hadoop.ozone.container.common.statemachine.DatanodeConfiguration;
+import org.apache.hadoop.ozone.container.common.statemachine.DatanodeStateMachine;
+import org.apache.hadoop.ozone.container.common.statemachine.StateContext;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainer;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.common.statemachine.commandhandler.DeleteBlocksCommandHandler.SchemaHandler;
+import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+import org.apache.hadoop.hdds.protocol.proto
+ .StorageContainerDatanodeProtocolProtos.ContainerBlocksDeletionACKProto
+ .DeleteBlockTransactionResult;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.Future;
+
+import static org.apache.hadoop.hdds.protocol.MockDatanodeDetails.randomDatanodeDetails;
+import static org.apache.hadoop.ozone.container.common.statemachine.commandhandler.DeleteBlocksCommandHandler.DeleteBlockTransactionExecutionResult;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V1;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V2;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V3;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test cases for TestDeleteBlocksCommandHandler.
+ */
+@RunWith(Parameterized.class)
+public class TestDeleteBlocksCommandHandler {
+
+ @Rule
+ public Timeout testTimeout = Timeout.seconds(300);
+
+ private OzoneConfiguration conf;
+ private StateContext context;
+ private ContainerLayoutVersion layout;
+ private OzoneContainer ozoneContainer;
+ private ContainerSet containerSet;
+ private DeleteBlocksCommandHandler handler;
+ private final String schemaVersion;
+ private HddsVolume volume1;
+ private BlockDeletingServiceMetrics blockDeleteMetrics;
+
+
+ public TestDeleteBlocksCommandHandler(String schemaVersion) {
+ this.schemaVersion = schemaVersion;
+ }
+
+ @Parameterized.Parameters(name = "{index}: Schema Version {0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][]{
+ {SCHEMA_V1},
+ {SCHEMA_V2},
+ {SCHEMA_V3},
+ });
+ }
Review Comment:
You can use `ContainerTestVersionInfo` to parameterize by schema version and layout version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1333822215
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -569,4 +661,25 @@ void apply(Table<?, DeletedBlocksTransaction> deleteTxnsTable,
BatchOperation batch, long txnID, DeletedBlocksTransaction delTX)
throws IOException;
}
+
+ /**
+ * The SchemaHandler interface provides a contract for handling
+ * KeyValueContainerData and DeletedBlocksTransaction based
+ * on different schema versions.
+ */
+ public interface SchemaHandler {
+ void handle(KeyValueContainerData containerData,
+ DeletedBlocksTransaction tx) throws IOException;
Review Comment:
@xichen01 , please correct the indent here. It's 4 blank space indent for a wrapped line. Same for TestSchemaHandler#handle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1330949770
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/BlockDeletingServiceMetrics.java:
##########
@@ -74,6 +74,10 @@ public final class BlockDeletingServiceMetrics {
@Metric(about = "The total number of Container chosen to be deleted.")
private MutableGaugeLong totalContainerChosenCount;
+ @Metric(about = "The total number of transactions which was failed " +
Review Comment:
which was failed cause by wait Container lock timeout -> which failed due to container lock wait timeout
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1234000475
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java:
##########
@@ -0,0 +1,283 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.hadoop.ozone.container.common.statemachine.commandhandler;
+
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.DeletedBlocksTransaction;
+import org.apache.hadoop.ozone.container.ContainerTestHelper;
+import org.apache.hadoop.ozone.container.common.helpers.BlockDeletingServiceMetrics;
+import org.apache.hadoop.ozone.container.common.impl.ContainerLayoutVersion;
+import org.apache.hadoop.ozone.container.common.impl.ContainerSet;
+import org.apache.hadoop.ozone.container.common.interfaces.Container;
+import org.apache.hadoop.ozone.container.common.statemachine.DatanodeStateMachine;
+import org.apache.hadoop.ozone.container.common.statemachine.StateContext;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainer;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import org.apache.hadoop.ozone.container.common.statemachine.commandhandler.DeleteBlocksCommandHandler.SchemaHandler;
+import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.jupiter.api.Timeout;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+import org.apache.hadoop.hdds.protocol.proto
+ .StorageContainerDatanodeProtocolProtos.ContainerBlocksDeletionACKProto
+ .DeleteBlockTransactionResult;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.Future;
+
+import static org.apache.hadoop.hdds.protocol.MockDatanodeDetails.randomDatanodeDetails;
+import static org.apache.hadoop.ozone.container.common.statemachine.commandhandler.DeleteBlocksCommandHandler.DeleteBlockTransactionExecutionResult;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V1;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V2;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V3;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test cases for TestDeleteBlocksCommandHandler.
+ */
+@RunWith(Parameterized.class)
+@Timeout(300)
Review Comment:
Nit: `@Timeout` is from JUnit5, all other test annotations are from JUnit4. I don't think timeout is enforced with this setup.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeStateMachine.java:
##########
@@ -226,7 +226,8 @@ public DatanodeStateMachine(DatanodeDetails datanodeDetails,
.addHandler(new CloseContainerCommandHandler())
.addHandler(new DeleteBlocksCommandHandler(getContainer(),
conf, dnConf.getBlockDeleteThreads(),
- dnConf.getBlockDeleteQueueLimit()))
+ dnConf.getBlockDeleteQueueLimit(),
+ dnConf.getBlockDeleteCommandHandleLockTimeoutMs()))
Review Comment:
Nit: pass `DatanodeConfiguration` to `DeleteBlocksCommandHandler` instead of individual values.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/BlockDeletingServiceMetrics.java:
##########
@@ -52,6 +52,10 @@ public final class BlockDeletingServiceMetrics {
@Metric(about = "The total number of blocks pending for processing.")
private MutableGaugeLong totalPendingBlockCount;
+ @Metric(about = "The total number of transactions witch was failed " +
+ "cause by wait Container lock timeout.")
Review Comment:
Typo: `witch` -> `which`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1331051495
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -232,40 +267,44 @@ public DeleteBlockTransactionResult call() {
ContainerType containerType = cont.getContainerType();
switch (containerType) {
case KeyValueContainer:
+ KeyValueContainer keyValueContainer = (KeyValueContainer)cont;
KeyValueContainerData containerData = (KeyValueContainerData)
cont.getContainerData();
- cont.writeLock();
- try {
- if (containerData.hasSchema(SCHEMA_V1)) {
- markBlocksForDeletionSchemaV1(containerData, tx);
- } else if (containerData.hasSchema(SCHEMA_V2)) {
- markBlocksForDeletionSchemaV2(containerData, tx,
- newDeletionBlocks, tx.getTxID());
- } else if (containerData.hasSchema(SCHEMA_V3)) {
- markBlocksForDeletionSchemaV3(containerData, tx,
- newDeletionBlocks, tx.getTxID());
- } else {
- throw new UnsupportedOperationException(
- "Only schema version 1,2,3 are supported.");
+ if (keyValueContainer.
+ writeLockTryLock(tryLockTimeoutMs, TimeUnit.MILLISECONDS)) {
+ try {
+ String schemaVersion = containerData
+ .getSupportedSchemaVersionOrDefault();
+ if (getSchemaHandlers().containsKey(schemaVersion)) {
+ schemaHandlers.get(schemaVersion).handle(containerData, tx);
+ } else {
+ throw new UnsupportedOperationException(
+ "Only schema version 1,2,3 are supported.");
+ }
+ } finally {
+ cont.writeUnlock();
}
- } finally {
- cont.writeUnlock();
+ txResultBuilder.setContainerID(containerId)
+ .setSuccess(true);
+ } else {
+ lockAcquisitionFailed = true;
+ txResultBuilder.setContainerID(containerId)
+ .setSuccess(false);
}
- txResultBuilder.setContainerID(containerId)
- .setSuccess(true);
break;
default:
LOG.error(
"Delete Blocks Command Handler is not implemented for " +
"containerType {}", containerType);
}
- } catch (IOException e) {
+ } catch (IOException | InterruptedException e) {
Review Comment:
Please call Thread.currentThread().interrupt() if it's an InterruptedException.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4913:
URL: https://github.com/apache/ozone/pull/4913#issuecomment-1731190355
The failed TestOzoneFileSystem.testListStatusOnKeyNameContainDelimiter is being addressed by HDDS-8877 #5093
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1330949353
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -188,6 +188,24 @@ public class DatanodeConfiguration {
private long recoveringContainerScrubInterval =
Duration.ofMinutes(10).toMillis();
+ /**
+ * Timeout for the thread used to process the delete block command
+ * to wait for the container lock.
+ * It takes about 200ms to open a RocksDB with HDD media.
+ * Set the default value to 100ms, so that after one times retry
+ * after waiting timeout, the hold time spent waiting for the lock
+ * is not greater than the time spent operating RocksDB
+ */
+ @Config(key = "block.delete.command.handle.lock.timeout",
Review Comment:
Suggest change the property name from block.delete.command.handle.lock.timeout ->
block.delete.max.lock.wait.timeout
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a diff in pull request #4913: HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4913:
URL: https://github.com/apache/ozone/pull/4913#discussion_r1331186578
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -188,6 +188,24 @@ public class DatanodeConfiguration {
private long recoveringContainerScrubInterval =
Duration.ofMinutes(10).toMillis();
+ /**
+ * Timeout for the thread used to process the delete block command
+ * to wait for the container lock.
+ * It takes about 200ms to open a RocksDB with HDD media.
+ * Set the default value to 100ms, so that after one times retry
+ * after waiting timeout, the hold time spent waiting for the lock
+ * is not greater than the time spent operating RocksDB
+ */
+ @Config(key = "block.delete.command.handle.lock.timeout",
Review Comment:
Will this property be a reconfigurable property?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org