nandakumar131 commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r812146058
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
##########
@@ -660,37 +640,14 @@ void checkContainerIsHealthy(KeyValueContainer
kvContainer, BlockID blockID,
/**
* Handle Delete Chunk operation. Calls ChunkManager to process the request.
*/
+ @Deprecated
Review comment:
+1 for deprecating.
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
##########
@@ -667,6 +689,47 @@ public long getBlockCommitSequenceId() {
return containerData.getBlockCommitSequenceId();
}
+ /**
+ * Return whether the given localID of a block is present in the
+ * pendingPutBlockCache or not.
+ */
+ public boolean isBlockInPendingPutBlockCache(long localID) {
+ if (pendingPutBlockCache != null) {
+ return pendingPutBlockCache.contains(localID);
+ } else {
+ pendingPutBlockCache = new ArrayList<>();
Review comment:
Why do we need to initialise `pendingPutBlockCache` here? Can't we just
return false?
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
##########
@@ -89,15 +90,30 @@
private final KeyValueContainerData containerData;
private ConfigurationSource config;
+ // Cache of Blocks (LocalIDs) awaiting final PutBlock call after the stream
+ // is closed. When a block is added to the DB as part of putBlock, it is
+ // added to the cache here. It is cleared from the Cache when the putBlock
+ // is called on the block as part of stream.close() (with endOfBlock = true
+ // in BlockManagerImpl#putBlock). Or when the container is marked for
+ // close, the whole cache is cleared as there can be no more writes to this
+ // container.
+ // We do not need to explicitly synchronize this cache as the writes to
+ // container are synchronous.
+ private ArrayList<Long> pendingPutBlockCache;
+
public KeyValueContainer(KeyValueContainerData containerData,
- ConfigurationSource
- ozoneConfig) {
+ ConfigurationSource ozoneConfig) {
Preconditions.checkNotNull(containerData,
"KeyValueContainerData cannot be null");
Preconditions.checkNotNull(ozoneConfig,
"Ozone configuration cannot be null");
this.config = ozoneConfig;
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.
+ this.pendingPutBlockCache = new ArrayList<>();
+ }
Review comment:
Suggestion: We can use `Collections.EMPTY_LIST` when the container is
not in open or closing state to avoid `null` checks later.
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
##########
@@ -1066,13 +1023,17 @@ public void deleteContainer(Container container,
boolean force)
deleteInternal(container, force);
}
+ /**
+ * Called by BlockDeletingService to delete all the chunks in a block
+ * before proceeding to delete the block info from DB.
+ */
@Override
public void deleteBlock(Container container, BlockData blockData)
throws IOException {
chunkManager.deleteChunks(container, blockData);
- for (ContainerProtos.ChunkInfo chunkInfo : blockData.getChunks()) {
- ChunkInfo info = ChunkInfo.getFromProtoBuf(chunkInfo);
- if (LOG.isDebugEnabled()) {
+ if (LOG.isDebugEnabled()) {
+ for (ContainerProtos.ChunkInfo chunkInfo : blockData.getChunks()) {
+ ChunkInfo info = ChunkInfo.getFromProtoBuf(chunkInfo);
Review comment:
Thanks for optimizing this code path.
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
##########
@@ -548,33 +547,14 @@ ContainerCommandResponseProto
handleGetCommittedBlockLength(
/**
* Handle Delete Block operation. Calls BlockManager to process the request.
*/
+ @Deprecated
Review comment:
+1 for deprecating.
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
##########
@@ -89,15 +90,30 @@
private final KeyValueContainerData containerData;
private ConfigurationSource config;
+ // Cache of Blocks (LocalIDs) awaiting final PutBlock call after the stream
+ // is closed. When a block is added to the DB as part of putBlock, it is
+ // added to the cache here. It is cleared from the Cache when the putBlock
+ // is called on the block as part of stream.close() (with endOfBlock = true
+ // in BlockManagerImpl#putBlock). Or when the container is marked for
+ // close, the whole cache is cleared as there can be no more writes to this
+ // container.
+ // We do not need to explicitly synchronize this cache as the writes to
+ // container are synchronous.
+ private ArrayList<Long> pendingPutBlockCache;
Review comment:
Suggestion: Since we are going to do lot of lookups on this cache (when
compared to add/remove), it's better to use Set/HashSet than ArrayList.
--
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]
