jojochuang commented on code in PR #5783:
URL: https://github.com/apache/ozone/pull/5783#discussion_r1428622457
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -562,6 +565,48 @@ ContainerCommandResponseProto handlePutBlock(
return putBlockResponseSuccess(request, blockDataProto);
}
+ ContainerCommandResponseProto handleFinalizeBlock(
+ ContainerCommandRequestProto request, KeyValueContainer kvContainer) {
+
+ if (!request.hasFinalizeBlock()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Malformed Finalize block request. trace ID: {}",
+ request.getTraceID());
+ }
+ return malformedRequest(request);
+ }
+ ContainerProtos.BlockData responseData;
+
+ try {
+ checkContainerOpen(kvContainer);
+ BlockID blockID = BlockID.getFromProtobuf(
+ request.getFinalizeBlock().getBlockID());
+ Preconditions.checkNotNull(blockID);
+
+ LOG.info("Finalized Block request received {} ", blockID);
+
+ responseData = blockManager.getBlock(kvContainer, blockID)
+ .getProtoBufMessage();
+
+ BlockData blockData = BlockData.getFromProtoBuf(responseData);
+
+ chunkManager.finalizeWriteChunk(kvContainer, blockData);
Review Comment:
You don't need blockData here. Just blockID.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -365,6 +367,20 @@ private static void populateContainerMetadata(
ContainerInspectorUtil.process(kvContainerData, store);
}
+ private static void populateContainerFinalizeBlock(
+ KeyValueContainerData kvContainerData, DatanodeStore store)
+ throws IOException {
+ Table<String, FinalizeBlockList> finalizeBlocksTable =
+ store.getFinalizeBlocksTable();
+
+ FinalizeBlockList finalizeBlockList =
+ finalizeBlocksTable.get(kvContainerData.getFinalizeBlockKey());
Review Comment:
I suspect you want to use KeyValueContainerData.getBlockKey() to add prefix
for different containers.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -562,6 +565,48 @@ ContainerCommandResponseProto handlePutBlock(
return putBlockResponseSuccess(request, blockDataProto);
}
+ ContainerCommandResponseProto handleFinalizeBlock(
+ ContainerCommandRequestProto request, KeyValueContainer kvContainer) {
+
+ if (!request.hasFinalizeBlock()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Malformed Finalize block request. trace ID: {}",
+ request.getTraceID());
+ }
+ return malformedRequest(request);
+ }
+ ContainerProtos.BlockData responseData;
+
+ try {
+ checkContainerOpen(kvContainer);
+ BlockID blockID = BlockID.getFromProtobuf(
+ request.getFinalizeBlock().getBlockID());
+ Preconditions.checkNotNull(blockID);
+
+ LOG.info("Finalized Block request received {} ", blockID);
+
+ responseData = blockManager.getBlock(kvContainer, blockID)
+ .getProtoBufMessage();
+
+ BlockData blockData = BlockData.getFromProtoBuf(responseData);
+
+ chunkManager.finalizeWriteChunk(kvContainer, blockData);
+ kvContainer.getContainerData()
+ .addToFinalizedBlockSet(blockData.getLocalID());
+ blockManager.finalizeBlock(kvContainer, blockData);
Review Comment:
You don't need blockData here. Just blockID.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerData.java:
##########
@@ -384,6 +413,10 @@ public String getDeletingBlockKeyPrefix() {
return formatKey(DELETING_KEY_PREFIX);
}
+ public String getFinalizeBlockKey() {
+ return formatKey("");
Review Comment:
missing a key here?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -376,8 +377,20 @@ public TransactionContext
startTransaction(RaftClientRequest request)
ctxt.setException(ioe);
return ctxt;
}
- if (proto.getCmdType() == Type.WriteChunk) {
+ if (proto.getCmdType() == Type.PutBlock) {
Review Comment:
Does PutBlock need change?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/interfaces/ChunkManager.java:
##########
@@ -106,6 +106,11 @@ default void finishWriteChunks(KeyValueContainer
kvContainer,
// no-op
}
+ default void finalizeWriteChunk(KeyValueContainer container,
+ BlockData blockData) throws IOException {
+ // no-op
Review Comment:
there's no corresponding implementation within FilePerChunkStrategy, which
is fine. I think that is a legacy code and doesn't make much sense for users to
enable that configuration. But it would be easier to troubleshoot if it throws
an exception instead of an no-op.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -365,6 +367,20 @@ private static void populateContainerMetadata(
ContainerInspectorUtil.process(kvContainerData, store);
}
+ private static void populateContainerFinalizeBlock(
+ KeyValueContainerData kvContainerData, DatanodeStore store)
+ throws IOException {
+ Table<String, FinalizeBlockList> finalizeBlocksTable =
+ store.getFinalizeBlocksTable();
+
+ FinalizeBlockList finalizeBlockList =
+ finalizeBlocksTable.get(kvContainerData.getFinalizeBlockKey());
Review Comment:
Here it looks a little strange. Assuming you fetch FinalizeBlockList
regardless of their containers, you would get all finalize blocks, but
FinalizeBlockList is a list of Long, it is unable to tell what containers the
blocks belong too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
