Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ public class DatanodeConfiguration {
* simultaneously.
*/
private int replicationMaxStreams = 10;
/**
* The maximum number of threads used to delete containers on a datanode
* simultaneously.
*/
private int deleteContainerThreads = 2;

@Config(key = "replication.streams.limit",
type = ConfigType.INT,
Expand All @@ -58,4 +63,25 @@ public int getReplicationMaxStreams() {
return replicationMaxStreams;
}

@Config(key = "delete.container.threads",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think container.delete.threads.max would better reflect both the hierarchical nature of config item naming, and the fact that it's an upper limit, not a fixed thread count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. It makes more sense to have "container.delete". I have changed this and also the related method names so they match up.

type = ConfigType.INT,
defaultValue = "2",
tags = {DATANODE},
description = "The maximum number of threads used to delete containers " +
"on a datanode"
)
public void setDeleteContainerThreads(int val) {
if (val < 1) {
LOG.warn("hdds.datanode.delete.container.threads must be greater than" +
"zero and was set to {}. Defaulting to {}",
val, deleteContainerThreads);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great only for the case when config is set during load from XML. Later, if the config is being set programmatically, we have two (minor) issues:

  1. the default value is not the real default, rather the previously set valid config
  2. warning is human-friendly, but irrelevant to programmatic callers

Eg.

setDeleteContainerThreads(10);
setDeleteContainerThreads(-1);

would log ... Defaulting to 10.

So I think this could be improved by:

  1. creating a constant for the default value
  2. using an Integer to distinguish between unset and set states
  3. logging the warning only if previously unset
  4. consider throwing an exception (or silently ignoring invalid values) if previously already set

(I wanted to mention this for the previous, replication-related PR, but was late to the party. ;) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are good points. For now, I have kept the warn log message, as I don't like the idea of silently ignoring bad values. From a usability perspective, I would like the DN to still start if an operator puts a bad value in the config rather than fail completely. However it is easy to argue this the other way too, that we should fail on a bad value rather than trying to be too clever.

I have taken on the default point and applied it to both parameters in the DatanodeConfig class.

} else {
this.deleteContainerThreads = val;
}
}

public int getDeleteContainerThreads() {
return deleteContainerThreads;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ public DatanodeStateMachine(DatanodeDetails datanodeDetails,
.addHandler(new DeleteBlocksCommandHandler(container.getContainerSet(),
conf))
.addHandler(new ReplicateContainerCommandHandler(conf, supervisor))
.addHandler(new DeleteContainerCommandHandler())
.addHandler(new DeleteContainerCommandHandler(
dnConf.getDeleteContainerThreads()))
.setConnectionManager(connectionManager)
.setContainer(container)
.setContext(context)
Expand Down Expand Up @@ -278,6 +279,10 @@ public void close() throws IOException {
if (jvmPauseMonitor != null) {
jvmPauseMonitor.stop();
}

if (commandDispatcher != null) {
commandDispatcher.stop();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ public void handle(SCMCommand command) {
}
}

public void stop() {
for (CommandHandler c : handlerMap.values()) {
c.stop();
}
}

public static Builder newBuilder() {
return new Builder();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,12 @@ default void updateCommandStatus(StateContext context, SCMCommand command,
command.getId());
}
}

/**
* Override for any command with an internal threadpool, and stop the
* executor when this method is invoked.
*/
default void stop() {
// Default implementation does nothing
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.hadoop.ozone.container.common.statemachine.commandhandler;

import com.google.common.util.concurrent.ThreadFactoryBuilder;
import org.apache.hadoop.hdds.protocol.proto
.StorageContainerDatanodeProtocolProtos.SCMCommandProto;
import org.apache.hadoop.ozone.container.common.statemachine
Expand All @@ -31,6 +32,11 @@
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;

/**
* Handler to process the DeleteContainerCommand from SCM.
Expand All @@ -40,28 +46,39 @@ public class DeleteContainerCommandHandler implements CommandHandler {
private static final Logger LOG =
LoggerFactory.getLogger(DeleteContainerCommandHandler.class);

private int invocationCount;
private long totalTime;
private final AtomicInteger invocationCount = new AtomicInteger(0);
private final AtomicLong totalTime = new AtomicLong(0);
private final ThreadPoolExecutor executor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer declaring it as the more generic ExecutorService.

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 have fixed this.


public DeleteContainerCommandHandler(int threadPoolSize) {
this.executor = new ThreadPoolExecutor(
0, threadPoolSize, 60, TimeUnit.SECONDS,
new LinkedBlockingQueue<>(),
new ThreadFactoryBuilder().setDaemon(true)
.setNameFormat("DeleteContainerThread-%d")
.build());
}

@Override
public void handle(final SCMCommand command,
final OzoneContainer ozoneContainer,
final StateContext context,
final SCMConnectionManager connectionManager) {
final long startTime = Time.monotonicNow();
invocationCount++;
try {
final DeleteContainerCommand deleteContainerCommand =
(DeleteContainerCommand) command;
final ContainerController controller = ozoneContainer.getController();
controller.deleteContainer(deleteContainerCommand.getContainerID(),
deleteContainerCommand.isForce());
} catch (IOException e) {
LOG.error("Exception occurred while deleting the container.", e);
} finally {
totalTime += Time.monotonicNow() - startTime;
}

final DeleteContainerCommand deleteContainerCommand =
(DeleteContainerCommand) command;
final ContainerController controller = ozoneContainer.getController();
executor.execute(() -> {
final long startTime = Time.monotonicNow();
invocationCount.incrementAndGet();
try {
controller.deleteContainer(deleteContainerCommand.getContainerID(),
deleteContainerCommand.isForce());
} catch (IOException e) {
LOG.error("Exception occurred while deleting the container.", e);
} finally {
totalTime.getAndAdd(Time.monotonicNow() - startTime);
}
});
}

@Override
Expand All @@ -71,11 +88,26 @@ public SCMCommandProto.Type getCommandType() {

@Override
public int getInvocationCount() {
return this.invocationCount;
return this.invocationCount.get();
}

@Override
public long getAverageRunTime() {
return invocationCount == 0 ? 0 : totalTime / invocationCount;
return invocationCount.get() == 0 ?
0 : totalTime.get() / invocationCount.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

The result of invocationCount.get() could be saved in a local variable for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted. I have fixed this.

}

@Override
public void stop() {
try {
executor.shutdown();
if (!executor.awaitTermination(3, TimeUnit.SECONDS)) {
executor.shutdownNow();
}
} catch (InterruptedException ie) {
// Ignore, we don't really care about the failure.
Thread.currentThread().interrupt();
}
}

}