chihsuan commented on code in PR #10967:
URL: https://github.com/apache/ozone/pull/10967#discussion_r3740368288
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -205,10 +205,17 @@ public long persistPutBlock(KeyValueContainer container,
return data.getSize();
}
- // Check if the block is present in the pendingPutBlockCache for the
- // container to determine whether the blockCount is already incremented
- // for this block in the DB or not.
long localID = data.getLocalID();
+
+ // PutBlock is not idempotent; ignore duplicate eof writes (HDDS-12007).
+ if (container.isBlockFinalizedByEof(localID)) {
Review Comment:
This check may be too late for a piggybacked `WriteChunk`,
since`KeyValueHandler` writes the chunk before calling putBlock.
https://github.com/apache/ozone/blob/a2e735d894c50a6b9187f8846248f9e8bf53a211/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java#L1131
The data may therefore already be changed when this check skips the metadata
update. Could the finalized-state check happen before chunk processing as well?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java:
##########
@@ -130,10 +139,12 @@ public KeyValueContainer(KeyValueContainerData
containerData,
this.containerData = containerData;
if (this.containerData.isOpen() || this.containerData.isClosing()) {
// If container is not in OPEN or CLOSING state, there cannot be block
- // writes to the container. So pendingPutBlockCache is not needed.
+ // writes to the container. So pendingPutBlockCache and eofBlockCache
are not needed.
this.pendingPutBlockCache = new HashSet<>();
+ this.eofBlockCache = new HashSet<>();
Review Comment:
This state is lost when a replica restarts. A later Raft entry could then be
skipped by replicas that retained the cache but persisted by the restarted
replica. Could the EOF state be persisted and restored with the container?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java:
##########
@@ -205,10 +205,17 @@ public long persistPutBlock(KeyValueContainer container,
return data.getSize();
}
- // Check if the block is present in the pendingPutBlockCache for the
- // container to determine whether the blockCount is already incremented
- // for this block in the DB or not.
long localID = data.getLocalID();
+
+ // PutBlock is not idempotent; ignore duplicate eof writes (HDDS-12007).
+ if (container.isBlockFinalizedByEof(localID)) {
+ LOG.warn("Ignoring write on block {} which has already been finalized "
+ + "by a PutBlock with the end-of-block flag set. PutBlock is not "
+ + "idempotent",
+ data.getBlockID());
+ return data.getSize();
Review Comment:
This skips the `PutBlock` but still reports success with its new BCS ID. The
client may then record a BCS ID that the datanode never persisted, causing
later reads to fail. Would it make sense to change this to throw exception with
`BLOCK_ALREADY_FINALIZED` instead?
--
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]
