You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/02/02 21:36:15 UTC
[GitHub] [ozone] hanishakoneru opened a new pull request #3034: Incorrect BLOCKCOUNT and BYTESUSED in container DB
hanishakoneru opened a new pull request #3034:
URL: https://github.com/apache/ozone/pull/3034
## What changes were proposed in this pull request?
Negative BlockCount and UsedBytes has been observed in ContainerDB.
The BlockCount is incremented only when the Stream is closed and not when the BlockID is added to the DB. If the OutputStream was not closed properly or if, for any reason, the client starts writing to a new pipeline before the full block is written, it could lead to a Block being present in the container but the key_count (BlockCount) not being incremented for it. When a block is deleted from a container, the blockCount is also decremented. But if the blockCount is wrong to start with, it could lead to a negative value.
When a block is deleted, usedBytes is decrement in memory first after deleting a chunk. And even if the chunkFile does not exist (already deleted), the usedBytes is decremented. This could lead to usedBytes being decremented multiple times for the same chunk and causing the total usedBytes metadata in the DB to become negative. Once all the chunks in all the blocks in that iteration of BlockDeletingService task are deleted, only then is the usedBytes updated in containerDB by taking the in-memory value. This Jira proposes to first update the DB with correct usedBytes (calculated from the BlockInfo after all chunks are deleted) and then update the in-memory metadata. This is the update sequence logic followed for all other state updates.
Also, when a chunk is overwritten, then it is assumed that the size of the chunk remains the same. But it’s possible to overwrite more data into the chunk than originally present. In this case, the used_bytes should be updated with difference in the chunkSizes. (Adding this as a TODO).
This PR has the following fixes:
1. Change Container DB KeyCount to BlockCount (to avoid confusion on what the keyCount denotes in ContainerData)
2. Fix BlockCount update logic
3. Fix UsedBytes decrement logic - update container DB first and then in-memory
4. Separate ContainerData metrics like read/write counts and bytes to different class
5. Cleanup and deprecate old Block and Chunk Delete methods
6. Add a test to verify blockCount and usedBytes are updated correctly on write failures
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5359
## How was this patch tested?
Added a 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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r801066168
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java
##########
@@ -415,41 +422,54 @@ void updateCommitInfo(XceiverClientReply reply, List<ChunkBuffer> buffers) {
ContainerCommandResponseProto> flushFuture = null;
try {
BlockData blockData = containerBlockData.build();
- XceiverClientReply asyncReply =
- putBlockAsync(xceiverClient, blockData, close, token);
+ boolean firstBlockPutCall = updateBlockCount.getAndSet(false);
+ XceiverClientReply asyncReply = putBlockAsync(xceiverClient, blockData,
+ firstBlockPutCall, close, token);
CompletableFuture<ContainerProtos.ContainerCommandResponseProto> future =
asyncReply.getResponse();
- flushFuture = future.thenApplyAsync(e -> {
+ flushFuture = future.thenApplyAsync(response -> {
try {
- validateResponse(e);
+ validateResponse(response);
} catch (IOException sce) {
throw new CompletionException(sce);
}
// if the ioException is not set, putBlock is successful
- if (getIoException() == null && !force) {
- BlockID responseBlockID = BlockID.getFromProtobuf(
- e.getPutBlock().getCommittedBlockLength().getBlockID());
- Preconditions.checkState(blockID.get().getContainerBlockID()
- .equals(responseBlockID.getContainerBlockID()));
- // updates the bcsId of the block
- blockID.set(responseBlockID);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Adding index " + asyncReply.getLogIndex() + " flushLength "
- + flushPos + " numBuffers " + byteBufferList.size()
- + " blockID " + blockID + " bufferPool size" + bufferPool
- .getSize() + " currentBufferIndex " + bufferPool
- .getCurrentBufferIndex());
+ if (getIoException() == null) {
+ if (!force) {
+ BlockID responseBlockID = BlockID.getFromProtobuf(
+ response.getPutBlock().getCommittedBlockLength().getBlockID());
+ Preconditions.checkState(blockID.get().getContainerBlockID()
+ .equals(responseBlockID.getContainerBlockID()));
+ // updates the bcsId of the block
+ blockID.set(responseBlockID);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Adding index " + asyncReply.getLogIndex() + " flushLength "
+ + flushPos + " numBuffers " + byteBufferList.size()
+ + " blockID " + blockID + " bufferPool size" + bufferPool
+ .getSize() + " currentBufferIndex " + bufferPool
+ .getCurrentBufferIndex());
+ }
+ // for standalone protocol, logIndex will always be 0.
+ updateCommitInfo(asyncReply, byteBufferList);
+ }
+ } else {
+ // PutBlock not successful -> reset updateBlockCount if it was set
+ // to true
+ if (firstBlockPutCall) {
+ updateBlockCount.compareAndSet(true, false);
Review comment:
Thanks @adoroszlai for catching this.
Yes, it should be set to true in case of failure.
--
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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r807096876
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -435,33 +449,45 @@ public ContainerBackgroundTaskResult deleteViaSchema2(
meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
}
}
+
+ // Handler.deleteBlock calls deleteChunk to delete all the chunks
+ // in the block. The ContainerData stats (DB and in-memory) are not
+ // updated with decremented used bytes during deleteChunk. This is
+ // done here so that all the DB update for block delete can be
+ // batched together while committing to DB.
meta.getStore().getBatchHandler().commitBatchOperation(batch);
containerData.updateAndCommitDBCounters(meta, batch,
- totalBlocks);
- // update count of pending deletion blocks and block count in
- // in-memory container status.
- containerData.decrPendingDeletionBlocks(totalBlocks);
- containerData.decrKeyCount(totalBlocks);
+ deletedBlocksCount, releasedBytes);
Review comment:
Thanks for catching this Ethan. I have removed it.
--
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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r801069862
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java
##########
@@ -143,11 +135,24 @@ public static long persistPutBlock(KeyValueContainer container,
containerBCSId, bcsId);
return data.getSize();
}
+
// update the blockData as well as BlockCommitSequenceId here
try (BatchOperation batch = db.getStore().getBatchHandler()
.initBatchOperation()) {
+ String dbKeyForBlock = Long.toString(data.getLocalID());
+
+ // If incrBlockCount is absent, then check the DB if the block exists
+ // or not. If it does not exist, increment the blockCount.
+ if (!incrBlockCount.isPresent()) {
+ if (db.getStore().getBlockDataTable().get(dbKeyForBlock) == null) {
+ incrBlockCount = Optional.of(false);
+ } else {
+ incrBlockCount = Optional.of(true);
+ }
+ }
Review comment:
It would be one extra read call to the DB for every putBlock call. So it would be better to use this information from client if it is available.
Also, realized that when key is null, we need to incrBlockCount (if else case needs to be reversed).
--
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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r801067544
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java
##########
@@ -89,18 +89,17 @@
//ID of the datanode where this container is created
private String originNodeId;
- /** parameters for read/write statistics on the container. **/
- private final AtomicLong readBytes;
- private final AtomicLong writeBytes;
- private final AtomicLong readCount;
- private final AtomicLong writeCount;
+ // Total number of blocks in the container
+ private final AtomicLong blockCount;
+ // Total number of bytes of all the blocks in the container
private final AtomicLong bytesUsed;
- private final AtomicLong keyCount;
private HddsVolume volume;
private String checksum;
+ private ContainerDataStats stats;
Review comment:
Not necessary for this fix. I will move it to different Jira.
--
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] errose28 commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r806297150
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -435,33 +449,45 @@ public ContainerBackgroundTaskResult deleteViaSchema2(
meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
}
}
+
+ // Handler.deleteBlock calls deleteChunk to delete all the chunks
+ // in the block. The ContainerData stats (DB and in-memory) are not
+ // updated with decremented used bytes during deleteChunk. This is
+ // done here so that all the DB update for block delete can be
+ // batched together while committing to DB.
meta.getStore().getBatchHandler().commitBatchOperation(batch);
containerData.updateAndCommitDBCounters(meta, batch,
- totalBlocks);
- // update count of pending deletion blocks and block count in
- // in-memory container status.
- containerData.decrPendingDeletionBlocks(totalBlocks);
- containerData.decrKeyCount(totalBlocks);
+ deletedBlocksCount, releasedBytes);
Review comment:
Do we need/want the extra commit on line 458?
--
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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r801066168
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java
##########
@@ -415,41 +422,54 @@ void updateCommitInfo(XceiverClientReply reply, List<ChunkBuffer> buffers) {
ContainerCommandResponseProto> flushFuture = null;
try {
BlockData blockData = containerBlockData.build();
- XceiverClientReply asyncReply =
- putBlockAsync(xceiverClient, blockData, close, token);
+ boolean firstBlockPutCall = updateBlockCount.getAndSet(false);
+ XceiverClientReply asyncReply = putBlockAsync(xceiverClient, blockData,
+ firstBlockPutCall, close, token);
CompletableFuture<ContainerProtos.ContainerCommandResponseProto> future =
asyncReply.getResponse();
- flushFuture = future.thenApplyAsync(e -> {
+ flushFuture = future.thenApplyAsync(response -> {
try {
- validateResponse(e);
+ validateResponse(response);
} catch (IOException sce) {
throw new CompletionException(sce);
}
// if the ioException is not set, putBlock is successful
- if (getIoException() == null && !force) {
- BlockID responseBlockID = BlockID.getFromProtobuf(
- e.getPutBlock().getCommittedBlockLength().getBlockID());
- Preconditions.checkState(blockID.get().getContainerBlockID()
- .equals(responseBlockID.getContainerBlockID()));
- // updates the bcsId of the block
- blockID.set(responseBlockID);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Adding index " + asyncReply.getLogIndex() + " flushLength "
- + flushPos + " numBuffers " + byteBufferList.size()
- + " blockID " + blockID + " bufferPool size" + bufferPool
- .getSize() + " currentBufferIndex " + bufferPool
- .getCurrentBufferIndex());
+ if (getIoException() == null) {
+ if (!force) {
+ BlockID responseBlockID = BlockID.getFromProtobuf(
+ response.getPutBlock().getCommittedBlockLength().getBlockID());
+ Preconditions.checkState(blockID.get().getContainerBlockID()
+ .equals(responseBlockID.getContainerBlockID()));
+ // updates the bcsId of the block
+ blockID.set(responseBlockID);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Adding index " + asyncReply.getLogIndex() + " flushLength "
+ + flushPos + " numBuffers " + byteBufferList.size()
+ + " blockID " + blockID + " bufferPool size" + bufferPool
+ .getSize() + " currentBufferIndex " + bufferPool
+ .getCurrentBufferIndex());
+ }
+ // for standalone protocol, logIndex will always be 0.
+ updateCommitInfo(asyncReply, byteBufferList);
+ }
+ } else {
+ // PutBlock not successful -> reset updateBlockCount if it was set
+ // to true
+ if (firstBlockPutCall) {
+ updateBlockCount.compareAndSet(true, false);
Review comment:
Thanks @adoroszlai for catching this.
Yes agree, it should be set to true in case of failure.
--
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] errose28 commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r806297150
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -435,33 +449,45 @@ public ContainerBackgroundTaskResult deleteViaSchema2(
meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
}
}
+
+ // Handler.deleteBlock calls deleteChunk to delete all the chunks
+ // in the block. The ContainerData stats (DB and in-memory) are not
+ // updated with decremented used bytes during deleteChunk. This is
+ // done here so that all the DB update for block delete can be
+ // batched together while committing to DB.
meta.getStore().getBatchHandler().commitBatchOperation(batch);
containerData.updateAndCommitDBCounters(meta, batch,
- totalBlocks);
- // update count of pending deletion blocks and block count in
- // in-memory container status.
- containerData.decrPendingDeletionBlocks(totalBlocks);
- containerData.decrKeyCount(totalBlocks);
+ deletedBlocksCount, releasedBytes);
Review comment:
Do we need/want the extra commit on line 458?
--
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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r812328316
##########
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:
In BlockManagerImpl#persistPutBlock, when we call isBlockInPendingPutBlockCache() and it returns false, it later on calls addToPendingPutBlockCache() to add the block to the cache.
In the good scenario, we should not call isBlockInPendingPutBlockCache() on a closed container. But I just added this check here to not introduce a NullPointerException in a highly unlikely exception corner case.
--
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 change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r798892524
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -29,6 +29,7 @@
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
+import javafx.util.Pair;
Review comment:
It seems `javafx.util` is not available in Java 11 by default. Can we use the one from `org.apache.commons.lang3.tuple` 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: 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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r812334902
##########
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:
Yes makes sense. I will update it. Thanks Nanda.
--
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] hanishakoneru commented on pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#issuecomment-1048467206
Thanks @nandakumar131 for the offline discussion. I updated the patch to remove the null check as discussed.
--
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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r801075744
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerDataStats.java
##########
@@ -0,0 +1,99 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.impl;
+
+import com.google.common.base.Preconditions;
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+
+public class ContainerDataStats {
Review comment:
Reverted the patch to separate the stats. Will open a new Jira for it. 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] adoroszlai commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r800180214
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java
##########
@@ -89,18 +89,17 @@
//ID of the datanode where this container is created
private String originNodeId;
- /** parameters for read/write statistics on the container. **/
- private final AtomicLong readBytes;
- private final AtomicLong writeBytes;
- private final AtomicLong readCount;
- private final AtomicLong writeCount;
+ // Total number of blocks in the container
+ private final AtomicLong blockCount;
+ // Total number of bytes of all the blocks in the container
private final AtomicLong bytesUsed;
- private final AtomicLong keyCount;
private HddsVolume volume;
private String checksum;
+ private ContainerDataStats stats;
Review comment:
Separating it is nice, but is it necessary for the fix? If not, can we separate it to a follow-up improvement Jira issue?
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerDataStats.java
##########
@@ -0,0 +1,99 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.impl;
+
+import com.google.common.base.Preconditions;
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+
+public class ContainerDataStats {
Review comment:
Would `ContainerOpStats` be a better name?
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java
##########
@@ -143,11 +135,24 @@ public static long persistPutBlock(KeyValueContainer container,
containerBCSId, bcsId);
return data.getSize();
}
+
// update the blockData as well as BlockCommitSequenceId here
try (BatchOperation batch = db.getStore().getBatchHandler()
.initBatchOperation()) {
+ String dbKeyForBlock = Long.toString(data.getLocalID());
+
+ // If incrBlockCount is absent, then check the DB if the block exists
+ // or not. If it does not exist, increment the blockCount.
+ if (!incrBlockCount.isPresent()) {
+ if (db.getStore().getBlockDataTable().get(dbKeyForBlock) == null) {
+ incrBlockCount = Optional.of(false);
+ } else {
+ incrBlockCount = Optional.of(true);
+ }
+ }
Review comment:
Given this fallback logic for old clients, is the client side change only a performance optimization? Would simply ignoring flag from client be a performance hit?
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java
##########
@@ -415,41 +422,54 @@ void updateCommitInfo(XceiverClientReply reply, List<ChunkBuffer> buffers) {
ContainerCommandResponseProto> flushFuture = null;
try {
BlockData blockData = containerBlockData.build();
- XceiverClientReply asyncReply =
- putBlockAsync(xceiverClient, blockData, close, token);
+ boolean firstBlockPutCall = updateBlockCount.getAndSet(false);
+ XceiverClientReply asyncReply = putBlockAsync(xceiverClient, blockData,
+ firstBlockPutCall, close, token);
CompletableFuture<ContainerProtos.ContainerCommandResponseProto> future =
asyncReply.getResponse();
- flushFuture = future.thenApplyAsync(e -> {
+ flushFuture = future.thenApplyAsync(response -> {
try {
- validateResponse(e);
+ validateResponse(response);
} catch (IOException sce) {
throw new CompletionException(sce);
}
// if the ioException is not set, putBlock is successful
- if (getIoException() == null && !force) {
- BlockID responseBlockID = BlockID.getFromProtobuf(
- e.getPutBlock().getCommittedBlockLength().getBlockID());
- Preconditions.checkState(blockID.get().getContainerBlockID()
- .equals(responseBlockID.getContainerBlockID()));
- // updates the bcsId of the block
- blockID.set(responseBlockID);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Adding index " + asyncReply.getLogIndex() + " flushLength "
- + flushPos + " numBuffers " + byteBufferList.size()
- + " blockID " + blockID + " bufferPool size" + bufferPool
- .getSize() + " currentBufferIndex " + bufferPool
- .getCurrentBufferIndex());
+ if (getIoException() == null) {
+ if (!force) {
+ BlockID responseBlockID = BlockID.getFromProtobuf(
+ response.getPutBlock().getCommittedBlockLength().getBlockID());
+ Preconditions.checkState(blockID.get().getContainerBlockID()
+ .equals(responseBlockID.getContainerBlockID()));
+ // updates the bcsId of the block
+ blockID.set(responseBlockID);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Adding index " + asyncReply.getLogIndex() + " flushLength "
+ + flushPos + " numBuffers " + byteBufferList.size()
+ + " blockID " + blockID + " bufferPool size" + bufferPool
+ .getSize() + " currentBufferIndex " + bufferPool
+ .getCurrentBufferIndex());
+ }
+ // for standalone protocol, logIndex will always be 0.
+ updateCommitInfo(asyncReply, byteBufferList);
+ }
+ } else {
+ // PutBlock not successful -> reset updateBlockCount if it was set
+ // to true
+ if (firstBlockPutCall) {
+ updateBlockCount.compareAndSet(true, false);
}
- // for standalone protocol, logIndex will always be 0.
- updateCommitInfo(asyncReply, byteBufferList);
}
- return e;
+ return response;
}, responseExecutor).exceptionally(e -> {
if (LOG.isDebugEnabled()) {
LOG.debug("putBlock failed for blockID {} with exception {}",
blockID, e.getLocalizedMessage());
}
+ // Reset updateBlockCount if it was set to true
+ if (firstBlockPutCall) {
+ updateBlockCount.compareAndSet(true, false);
Review comment:
By the same rationale:
```suggestion
// Reset updateBlockCount if it was set to false
if (firstBlockPutCall) {
updateBlockCount.set(true);
```
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java
##########
@@ -415,41 +422,54 @@ void updateCommitInfo(XceiverClientReply reply, List<ChunkBuffer> buffers) {
ContainerCommandResponseProto> flushFuture = null;
try {
BlockData blockData = containerBlockData.build();
- XceiverClientReply asyncReply =
- putBlockAsync(xceiverClient, blockData, close, token);
+ boolean firstBlockPutCall = updateBlockCount.getAndSet(false);
+ XceiverClientReply asyncReply = putBlockAsync(xceiverClient, blockData,
+ firstBlockPutCall, close, token);
CompletableFuture<ContainerProtos.ContainerCommandResponseProto> future =
asyncReply.getResponse();
- flushFuture = future.thenApplyAsync(e -> {
+ flushFuture = future.thenApplyAsync(response -> {
try {
- validateResponse(e);
+ validateResponse(response);
} catch (IOException sce) {
throw new CompletionException(sce);
}
// if the ioException is not set, putBlock is successful
- if (getIoException() == null && !force) {
- BlockID responseBlockID = BlockID.getFromProtobuf(
- e.getPutBlock().getCommittedBlockLength().getBlockID());
- Preconditions.checkState(blockID.get().getContainerBlockID()
- .equals(responseBlockID.getContainerBlockID()));
- // updates the bcsId of the block
- blockID.set(responseBlockID);
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "Adding index " + asyncReply.getLogIndex() + " flushLength "
- + flushPos + " numBuffers " + byteBufferList.size()
- + " blockID " + blockID + " bufferPool size" + bufferPool
- .getSize() + " currentBufferIndex " + bufferPool
- .getCurrentBufferIndex());
+ if (getIoException() == null) {
+ if (!force) {
+ BlockID responseBlockID = BlockID.getFromProtobuf(
+ response.getPutBlock().getCommittedBlockLength().getBlockID());
+ Preconditions.checkState(blockID.get().getContainerBlockID()
+ .equals(responseBlockID.getContainerBlockID()));
+ // updates the bcsId of the block
+ blockID.set(responseBlockID);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Adding index " + asyncReply.getLogIndex() + " flushLength "
+ + flushPos + " numBuffers " + byteBufferList.size()
+ + " blockID " + blockID + " bufferPool size" + bufferPool
+ .getSize() + " currentBufferIndex " + bufferPool
+ .getCurrentBufferIndex());
+ }
+ // for standalone protocol, logIndex will always be 0.
+ updateCommitInfo(asyncReply, byteBufferList);
+ }
+ } else {
+ // PutBlock not successful -> reset updateBlockCount if it was set
+ // to true
+ if (firstBlockPutCall) {
+ updateBlockCount.compareAndSet(true, false);
Review comment:
This doesn't seem right.
1. Comparison is unnecessary. `updateBlockCount` can only have two values, `true` or `false`. The `compareAndSet` call sets it to `false` if currently `true`, otherwise (if it's already `false`) it does not change it. The end result is that `updateBlockCount` is always `false` at the end. Thus we can directly `set(false)` without checking current value. I think `compareAndSet` is useful only if we use its return value, which reflects whether it has made the update or not.
2. More importantly the expected and updated values seem to be swapped. `updateBlockCount` starts out as `true`, but by the time we got here, it must have been set to `false`. Isn't this intended to reset to the original value, which is `true`?
(Hope that makes sense, let me know if I should further clarify.)
```suggestion
// PutBlock not successful -> reset updateBlockCount if it was set
// to true
if (firstBlockPutCall) {
updateBlockCount.set(true);
```
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestFailureHandlingByClient.java
##########
@@ -201,8 +207,78 @@ public void testBlockWritesWithDnFailures() throws Exception {
.setRefreshPipeline(true)
.build();
OmKeyInfo keyInfo = cluster.getOzoneManager().lookupKey(keyArgs);
+
Assert.assertEquals(data.length, keyInfo.getDataSize());
validateData(keyName, data);
+
+ // Verify that the block information is updated correctly in the DB on
+ // failures
+ testBlockCountOnFailures(keyInfo);
+ }
+
+ /**
+ * Test whether blockData and Container metadata (block count and used
+ * bytes) is updated correctly when there is a write failure.
+ * We can combine this test with {@link #testBlockWritesWithDnFailures()}
+ * as that test also simulates a write failure and client writes failed
+ * chunk writes to a new block.
+ */
+ private void testBlockCountOnFailures(OmKeyInfo omKeyInfo) throws Exception {
+ // testBlockWritesWithDnFailures writes chunkSize*1.5 size of data into
+ // KeyOutputStream. But before closing the outputStream, 2 of the DNs in
+ // the pipeline being written to are closed. This forces the client to
+ // write the uncommitted data in the buffer (first chunkSize of data will
+ // be committed before close as the flush size = chunkSize, the last 0
+ // .5*chunkSize of data will be in buffer) to another block in a
+ // different pipeline.
+
+ // Get information about the first and second block (in different pipelines)
+ List<OmKeyLocationInfo> locationList = omKeyInfo.getLatestVersionLocations()
+ .getLocationList();
+ long containerId1 = locationList.get(0).getContainerID();
+ long containerId2 = locationList.get(1).getContainerID();
Review comment:
This is failing with `IndexOutOfBoundsException: Index: 1, Size: 1`.
--
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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r799657279
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -29,6 +29,7 @@
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
+import javafx.util.Pair;
Review comment:
Done. Thanks @adoroszlai.
--
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] hanishakoneru edited a comment on pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru edited a comment on pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#issuecomment-1031947971
Thanks @adoroszlai for reviewing the patch. I addressed some of your comments.
I am looking into the test failure. It passes locally for me.
--
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] hanishakoneru commented on pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#issuecomment-1043284150
Thanks @nandakumar131 for the offline discussion.
As you suggested, I am initializing KeyValueContainer#PendingPutBlockCache only if the containerState is OPEN or CLOSING. Please take a look.
There is no possibility that a container can go back to open state from any of the other states, right? To be on the safer side, we can add a null check before adding/ removing from PendingPutBlockCache. Let me know what you think. 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] nandakumar131 commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
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: 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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r812333779
##########
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:
The null check is a safety net I added just in case there is a scenario (not that I can think of right now) where we try to call add/ remove on an uninitialized object.
How about I add a TODO to remove the null check (and replace with empty collection)?
--
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] hanishakoneru commented on pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#issuecomment-1031947971
Thanks @adoroszlai for reviewing the patch. I addressed some of your comments.
I am looking into the test failure.
--
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] hanishakoneru commented on a change in pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3034:
URL: https://github.com/apache/ozone/pull/3034#discussion_r807096876
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -435,33 +449,45 @@ public ContainerBackgroundTaskResult deleteViaSchema2(
meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
}
}
+
+ // Handler.deleteBlock calls deleteChunk to delete all the chunks
+ // in the block. The ContainerData stats (DB and in-memory) are not
+ // updated with decremented used bytes during deleteChunk. This is
+ // done here so that all the DB update for block delete can be
+ // batched together while committing to DB.
meta.getStore().getBatchHandler().commitBatchOperation(batch);
containerData.updateAndCommitDBCounters(meta, batch,
- totalBlocks);
- // update count of pending deletion blocks and block count in
- // in-memory container status.
- containerData.decrPendingDeletionBlocks(totalBlocks);
- containerData.decrKeyCount(totalBlocks);
+ deletedBlocksCount, releasedBytes);
Review comment:
Thanks for catching this Ethan. I have removed it.
--
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] nandakumar131 merged pull request #3034: HDDS-5359. Incorrect BLOCKCOUNT and BYTESUSED in container DB
Posted by GitBox <gi...@apache.org>.
nandakumar131 merged pull request #3034:
URL: https://github.com/apache/ozone/pull/3034
--
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