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 2020/12/15 12:20:18 UTC
[GitHub] [ozone] aryangupta1998 opened a new pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
aryangupta1998 opened a new pull request #1702:
URL: https://github.com/apache/ozone/pull/1702
## What changes were proposed in this pull request?
Currently datanode breaks down the transaction into individual blocks and then stores every block as separate entry in rocksDB. Since transaction can contain multiple blocks it is better to store the entire transaction in rocksDB.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4369
## How was this patch tested?
Tested Manually.
----------------------------------------------------------------
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.
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] lokeshj1703 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548444428
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -252,7 +252,8 @@ private OzoneConsts() {
// versions, requiring this property to be tracked on a per container basis.
// V1: All data in default column family.
public static final String SCHEMA_V1 = "1";
- // V2: Metadata, block data, and deleted blocks in their own column families.
+ // V2: Metadata, block data, and transactions to be deleted in their own
Review comment:
NIT: Let's rename "transactions to be deleted" to "delete transactions".
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -82,7 +89,9 @@
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_LIMIT_PER_CONTAINER;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_LIMIT_PER_CONTAINER_DEFAULT;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_SERVICE_INTERVAL;
+import static org.apache.hadoop.ozone.OzoneConsts.*;
Review comment:
Star import.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+ }
+
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ int deleteBlockCount =
+ deleteTransaction(delBlocks, handler, blockDataTable, container);
+
+ // Once blocks are deleted... remove the blockID from blockDataTable
+ // and also remove the transactions from txnTable.
+ try(BatchOperation batch = meta.getStore().getBatchHandler()
+ .initBatchOperation()) {
+ for (DeletedBlocksTransaction delTx : delBlocks) {
+ deleteTxns.deleteWithBatch(batch, delTx.getTxID());
+ for (Long blk : delTx.getLocalIDList()) {
+ String bID = blk.toString();
+ meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
+ }
+ meta.getStore().getBatchHandler().commitBatchOperation(batch);
Review comment:
commit operation should be outside the for loop.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+ }
+
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
Review comment:
Since this is also common code between both schemas, lets extract it out to a separate function like checkDataDir. I think this should be the first function to be called.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
Review comment:
we can return crr here as well.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+ }
+
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ int deleteBlockCount =
+ deleteTransaction(delBlocks, handler, blockDataTable, container);
+
+ // Once blocks are deleted... remove the blockID from blockDataTable
+ // and also remove the transactions from txnTable.
+ try(BatchOperation batch = meta.getStore().getBatchHandler()
+ .initBatchOperation()) {
+ for (DeletedBlocksTransaction delTx : delBlocks) {
+ deleteTxns.deleteWithBatch(batch, delTx.getTxID());
+ for (Long blk : delTx.getLocalIDList()) {
+ String bID = blk.toString();
+ meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
+ }
+ meta.getStore().getBatchHandler().commitBatchOperation(batch);
+ }
+ containerData.updateAndCommitDBCounters(meta, batch,
+ deleteBlockCount);
+ // update count of pending deletion blocks and block count in
+ // in-memory container status.
+ containerData.decrPendingDeletionBlocks(deleteBlockCount);
+ containerData.decrKeyCount(deleteBlockCount);
+ }
+
+ if (deleteBlockCount > 0) {
+ LOG.info("Container: {}, deleted blocks: {}, task elapsed time: {}ms",
+ containerData.getContainerID(), deleteBlockCount,
+ Time.monotonicNow() - startTime);
+ }
+ return crr;
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ private int deleteTransaction(List<DeletedBlocksTransaction> delBlocks,
Review comment:
NIT: Rename to deleteTransactions.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -242,52 +269,73 @@ private void deleteKeyValueContainerBlocks(
LOG.debug("Transited Block {} to DELETING state in container {}",
blk, containerId);
}
- } catch (IOException e) {
- // if some blocks failed to delete, we fail this TX,
- // without sending this ACK to SCM, SCM will resend the TX
- // with a certain number of retries.
- throw new IOException(
- "Failed to delete blocks for TXID = " + delTX.getTxID(), e);
- }
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Block {} not found or already under deletion in"
- + " container {}, skip deleting it.", blk, containerId);
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Block {} not found or already under deletion in"
+ + " container {}, skip deleting it.", blk, containerId);
+ }
}
}
+ updateMetaData(containerData, delTX, newDeletionBlocks, containerDB,
+ batch);
Review comment:
commitBatchOperation should be after updateMetadata call. Otherwise these operations would not be committed to DB.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+ }
+
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ int deleteBlockCount =
+ deleteTransaction(delBlocks, handler, blockDataTable, container);
Review comment:
We can decrease the pendingBlocks and other metadata by the number of blocks in the txns itself. Handling cases where few blocks are deleted can be done in a separate jira. For now it can be best effort to delete blocks.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r552391032
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+
+ int deleteBlockCount = 0;
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ for (DeletedBlocksTransaction entry: delBlocks) {
Review comment:
I will create a separate Jira for the partial blocks deletion issue.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548499058
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+ }
+
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
Review comment:
Made a common function for this code.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547488066
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManagerHelper.java
##########
@@ -137,6 +143,29 @@ public TestStorageContainerManagerHelper(MiniOzoneCluster cluster,
return allBlocks;
}
+ public boolean verifyBlocksWithTxnTable(Map<Long, List<Long>> containerBlocks)
+ throws IOException {
+ Set<Long> containerIDs = containerBlocks.keySet();
+ for (Long entry : containerIDs) {
+ ReferenceCountedDB meta = getContainerMetadata(entry);
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ List<? extends Table.KeyValue<Long, DeletedBlocksTransaction>> kvs =
+ dnStoreTwoImpl.getTxnTable()
+ .getRangeKVs(null, Integer.MAX_VALUE, null);
+ List<Long> conID = new ArrayList<>();
Review comment:
Renamed it blocksInTxnTable.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548499202
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+ }
+
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ int deleteBlockCount =
+ deleteTransaction(delBlocks, handler, blockDataTable, container);
+
+ // Once blocks are deleted... remove the blockID from blockDataTable
+ // and also remove the transactions from txnTable.
+ try(BatchOperation batch = meta.getStore().getBatchHandler()
+ .initBatchOperation()) {
+ for (DeletedBlocksTransaction delTx : delBlocks) {
+ deleteTxns.deleteWithBatch(batch, delTx.getTxID());
+ for (Long blk : delTx.getLocalIDList()) {
+ String bID = blk.toString();
+ meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
+ }
+ meta.getStore().getBatchHandler().commitBatchOperation(batch);
Review comment:
Moved commit operation outside the for loop.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547467624
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -289,8 +291,7 @@ public void testBlockDeletionTransactions() throws Exception {
return false;
}
}, 1000, 10000);
- Assert.assertTrue(helper.getAllBlocks(containerIDs).isEmpty());
-
+ Assert.assertTrue(helper.verifyBlocksWithTxnTable(containerBlocks));
Review comment:
As of now @errose28, TestStorageContainerManager uses SCHEMA version 2 so we don't need separate functions for schema 1 and schema 2. But yes in follow-up Jira we can make changes in TestStorageContainerManager so that it runs on both the schema versions and then we can add functions schema version-wise.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548499488
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+ }
+
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ int deleteBlockCount =
+ deleteTransaction(delBlocks, handler, blockDataTable, container);
+
+ // Once blocks are deleted... remove the blockID from blockDataTable
+ // and also remove the transactions from txnTable.
+ try(BatchOperation batch = meta.getStore().getBatchHandler()
+ .initBatchOperation()) {
+ for (DeletedBlocksTransaction delTx : delBlocks) {
+ deleteTxns.deleteWithBatch(batch, delTx.getTxID());
+ for (Long blk : delTx.getLocalIDList()) {
+ String bID = blk.toString();
+ meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
+ }
+ meta.getStore().getBatchHandler().commitBatchOperation(batch);
+ }
+ containerData.updateAndCommitDBCounters(meta, batch,
+ deleteBlockCount);
+ // update count of pending deletion blocks and block count in
+ // in-memory container status.
+ containerData.decrPendingDeletionBlocks(deleteBlockCount);
+ containerData.decrKeyCount(deleteBlockCount);
+ }
+
+ if (deleteBlockCount > 0) {
+ LOG.info("Container: {}, deleted blocks: {}, task elapsed time: {}ms",
+ containerData.getContainerID(), deleteBlockCount,
+ Time.monotonicNow() - startTime);
+ }
+ return crr;
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ private int deleteTransaction(List<DeletedBlocksTransaction> delBlocks,
Review comment:
Renamed it deleteTransactions.
----------------------------------------------------------------
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.
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] lokeshj1703 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r544301751
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
+ for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ BlockID blockID =
+ ContainerTestHelper.getTestBlockID(containerID);
+ String deleteStateName = OzoneConsts.DELETING_KEY_PREFIX +
+ blockID.getLocalID();
+ BlockData kd = new BlockData(blockID);
+ List<ContainerProtos.ChunkInfo> chunks = Lists.newArrayList();
+ putChunksInBlock(numOfChunksPerBlock, j, chunks, buffer,
+ chunkManager, container, blockID);
+ kd.setChunks(chunks);
+ metadata.getStore().getBlockDataTable().put(
+ deleteStateName, kd);
+ container.getContainerData().incrPendingDeletionBlocks(1);
+ }
+ updateMetaData(data, container, numOfBlocksPerContainer,
+ numOfChunksPerBlock);
+ }
+ } else if (data.getSchemaVersion().equals(SCHEMA_V2)) {
+ Map<Long, List<Long>> containerBlocks = new HashMap<>();
+ int blockCount = 0;
+ for (int i = 0; i < numOfBlocksPerContainer; i++) {
+ txnID = txnID + 1;
BlockID blockID =
ContainerTestHelper.getTestBlockID(containerID);
- String deleteStateName = OzoneConsts.DELETING_KEY_PREFIX +
- blockID.getLocalID();
BlockData kd = new BlockData(blockID);
List<ContainerProtos.ChunkInfo> chunks = Lists.newArrayList();
- for (int k = 0; k < numOfChunksPerBlock; k++) {
- final String chunkName = String.format("block.%d.chunk.%d", j, k);
- final long offset = k * chunkLength;
- ContainerProtos.ChunkInfo info =
- ContainerProtos.ChunkInfo.newBuilder()
- .setChunkName(chunkName)
- .setLen(chunkLength)
- .setOffset(offset)
- .setChecksumData(Checksum.getNoChecksumDataProto())
- .build();
- chunks.add(info);
- ChunkInfo chunkInfo = new ChunkInfo(chunkName, offset, chunkLength);
- ChunkBuffer chunkData = buffer.duplicate(0, (int) chunkLength);
- chunkManager.writeChunk(container, blockID, chunkInfo, chunkData,
- WRITE_STAGE);
- chunkManager.writeChunk(container, blockID, chunkInfo, chunkData,
- COMMIT_STAGE);
- }
+ putChunksInBlock(numOfChunksPerBlock, i, chunks, buffer, chunkManager,
+ container, blockID);
kd.setChunks(chunks);
- metadata.getStore().getBlockDataTable().put(
- deleteStateName, kd);
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
+ String bID = blockID.getLocalID()+"";
+ metadata.getStore().getBlockDataTable().put(bID, kd);
+ }
container.getContainerData().incrPendingDeletionBlocks(1);
+
+ // In below if statements we are checking if a single container
+ // consists of more blocks than 'blockLimitPerTask' then we create
+ // (totalBlocksInContainer / blockLimitPerTask) transactions which
+ // consists of blocks equal to blockLimitPerTask and last transaction
+ // consists of blocks equal to
+ // (totalBlocksInContainer % blockLimitPerTask).
+ if (blockCount < blockLimitPerTask) {
+ if (containerBlocks.containsKey(containerID)) {
+ containerBlocks.get(containerID).add(blockID.getLocalID());
+ } else {
+ List<Long> item = new ArrayList<>();
+ item.add(blockID.getLocalID());
+ containerBlocks.put(containerID, item);
+ }
+ blockCount++;
+ }
+ boolean flag = false;
+ if (blockCount == blockLimitPerTask) {
+ createTxn(data, containerBlocks, txnID);
+ containerBlocks.clear();
+ blockCount = 0;
+ flag = true;
+ }
+ if (i == (numOfBlocksPerContainer - 1) && !flag) {
+ createTxn(data, containerBlocks, txnID);
+ }
Review comment:
```suggestion
containerBlocks.add(blockID.getLocalID());
blockCount++;
if (blockCount == blockLimitPerTask || i == (numOfBlocksPerContainer - 1)) {
createTxn(data, containerBlocks, txnID);
containerBlocks.clear();
blockCount = 0;
}
```
I think we can simplify the code here. We can use a List for storing containerBlocks.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548499588
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -82,7 +89,9 @@
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_LIMIT_PER_CONTAINER;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_LIMIT_PER_CONTAINER_DEFAULT;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_SERVICE_INTERVAL;
+import static org.apache.hadoop.ozone.OzoneConsts.*;
Review comment:
Replaced star with necessary imports.
----------------------------------------------------------------
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.
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] lokeshj1703 commented on pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#issuecomment-755131776
@aryangupta1998 Thanks for the contribution! @errose28 Thanks for the review! I have committed the PR to master branch.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548498664
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -252,7 +252,8 @@ private OzoneConsts() {
// versions, requiring this property to be tracked on a per container basis.
// V1: All data in default column family.
public static final String SCHEMA_V1 = "1";
- // V2: Metadata, block data, and deleted blocks in their own column families.
+ // V2: Metadata, block data, and transactions to be deleted in their own
Review comment:
Renamed it delete transactions.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547486718
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -129,7 +134,16 @@ public void handle(SCMCommand command, OzoneContainer container,
cont.getContainerData();
cont.writeLock();
try {
- deleteKeyValueContainerBlocks(containerData, entry);
+ if (containerData.getSchemaVersion().equals(SCHEMA_V1)) {
+ deleteKeyValueContainerBlocks(containerData, entry);
+ } else if (containerData.getSchemaVersion().equals(SCHEMA_V2)) {
+ putTxns(containerData, entry, newDeletionBlocks,
Review comment:
Renamed Accordingly.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547436208
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
Review comment:
Thanks for pointing this out @errose28. It can violate the blockLimitPerTask here if totalBlocks + delTx.getLocalIDList().size() > blockLimitPerTask but this would handle a case when a transaction contains blocks > blockLimitPerTask. If we would put a check to blockLimitPerTask inside while loop then the transaction which contains blocks > blockLimitPerTask will be completely ignored and will never be deleted. We also have some tests in testBlockDeletingService.java where the container contains blocks more than blockLimitPerTask.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r551909889
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
Review comment:
Done
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r551959888
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
Review comment:
Done
----------------------------------------------------------------
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.
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 pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
errose28 commented on pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#issuecomment-754786912
LGTM @aryangupta1998 +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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547485893
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
+ for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ BlockID blockID =
+ ContainerTestHelper.getTestBlockID(containerID);
+ String deleteStateName = OzoneConsts.DELETING_KEY_PREFIX +
+ blockID.getLocalID();
+ BlockData kd = new BlockData(blockID);
+ List<ContainerProtos.ChunkInfo> chunks = Lists.newArrayList();
+ putChunksInBlock(numOfChunksPerBlock, j, chunks, buffer,
+ chunkManager, container, blockID);
+ kd.setChunks(chunks);
+ metadata.getStore().getBlockDataTable().put(
+ deleteStateName, kd);
+ container.getContainerData().incrPendingDeletionBlocks(1);
+ }
+ updateMetaData(data, container, numOfBlocksPerContainer,
+ numOfChunksPerBlock);
+ }
+ } else if (data.getSchemaVersion().equals(SCHEMA_V2)) {
+ Map<Long, List<Long>> containerBlocks = new HashMap<>();
+ int blockCount = 0;
+ for (int i = 0; i < numOfBlocksPerContainer; i++) {
+ txnID = txnID + 1;
BlockID blockID =
ContainerTestHelper.getTestBlockID(containerID);
- String deleteStateName = OzoneConsts.DELETING_KEY_PREFIX +
- blockID.getLocalID();
BlockData kd = new BlockData(blockID);
List<ContainerProtos.ChunkInfo> chunks = Lists.newArrayList();
- for (int k = 0; k < numOfChunksPerBlock; k++) {
- final String chunkName = String.format("block.%d.chunk.%d", j, k);
- final long offset = k * chunkLength;
- ContainerProtos.ChunkInfo info =
- ContainerProtos.ChunkInfo.newBuilder()
- .setChunkName(chunkName)
- .setLen(chunkLength)
- .setOffset(offset)
- .setChecksumData(Checksum.getNoChecksumDataProto())
- .build();
- chunks.add(info);
- ChunkInfo chunkInfo = new ChunkInfo(chunkName, offset, chunkLength);
- ChunkBuffer chunkData = buffer.duplicate(0, (int) chunkLength);
- chunkManager.writeChunk(container, blockID, chunkInfo, chunkData,
- WRITE_STAGE);
- chunkManager.writeChunk(container, blockID, chunkInfo, chunkData,
- COMMIT_STAGE);
- }
+ putChunksInBlock(numOfChunksPerBlock, i, chunks, buffer, chunkManager,
+ container, blockID);
kd.setChunks(chunks);
- metadata.getStore().getBlockDataTable().put(
- deleteStateName, kd);
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
+ String bID = blockID.getLocalID()+"";
+ metadata.getStore().getBlockDataTable().put(bID, kd);
+ }
container.getContainerData().incrPendingDeletionBlocks(1);
+
+ // In below if statements we are checking if a single container
+ // consists of more blocks than 'blockLimitPerTask' then we create
+ // (totalBlocksInContainer / blockLimitPerTask) transactions which
+ // consists of blocks equal to blockLimitPerTask and last transaction
+ // consists of blocks equal to
+ // (totalBlocksInContainer % blockLimitPerTask).
+ if (blockCount < blockLimitPerTask) {
+ if (containerBlocks.containsKey(containerID)) {
+ containerBlocks.get(containerID).add(blockID.getLocalID());
+ } else {
+ List<Long> item = new ArrayList<>();
+ item.add(blockID.getLocalID());
+ containerBlocks.put(containerID, item);
+ }
+ blockCount++;
+ }
+ boolean flag = false;
+ if (blockCount == blockLimitPerTask) {
+ createTxn(data, containerBlocks, txnID);
+ containerBlocks.clear();
+ blockCount = 0;
+ flag = true;
+ }
+ if (i == (numOfBlocksPerContainer - 1) && !flag) {
+ createTxn(data, containerBlocks, txnID);
+ }
Review comment:
Thanks for this suggestion, made the necessary changes.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548499339
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
Review comment:
Added the return statement here as well.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547468856
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
+ KeyValueContainerData containerData, DeletedBlocksTransaction delTX,
+ int newDeletionBlocks) throws IOException {
+ long containerId = delTX.getContainerID();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Processing Container : {}, DB path : {}", containerId,
+ containerData.getMetadataPath());
+ }
+
+ if (delTX.getTxID() < containerData.getDeleteTransactionId()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(String.format("Ignoring delete blocks for containerId: %d."
+ + " Outdated delete transactionId %d < %d", containerId,
+ delTX.getTxID(), containerData.getDeleteTransactionId()));
+ }
+ return;
+ }
+ try(ReferenceCountedDB containerDB =
+ BlockUtils.getDB(containerData, conf)) {
+
+ if (newDeletionBlocks > 0) {
+ // Finally commit the DB counters.
+ try(BatchOperation batchOperation =
+ containerDB.getStore().getBatchHandler().initBatchOperation()) {
+ Table< String, Long > metadataTable = containerDB.getStore()
+ .getMetadataTable();
+
+ // In memory is updated only when existing delete transactionID is
+ // greater.
+ if (delTX.getTxID() > containerData.getDeleteTransactionId()) {
+ // Update in DB pending delete key count and delete transaction ID.
+ metadataTable.putWithBatch(batchOperation,
+ OzoneConsts.DELETE_TRANSACTION_KEY, delTX.getTxID());
+ }
+
+ long pendingDeleteBlocks =
+ containerData.getNumPendingDeletionBlocks() + newDeletionBlocks;
+ metadataTable.putWithBatch(batchOperation,
+ OzoneConsts.PENDING_DELETE_BLOCK_COUNT, pendingDeleteBlocks);
Review comment:
Addressed the same in the above comment.
----------------------------------------------------------------
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.
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] lokeshj1703 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r544178673
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -129,7 +134,16 @@ public void handle(SCMCommand command, OzoneContainer container,
cont.getContainerData();
cont.writeLock();
try {
- deleteKeyValueContainerBlocks(containerData, entry);
+ if (containerData.getSchemaVersion().equals(SCHEMA_V1)) {
+ deleteKeyValueContainerBlocks(containerData, entry);
+ } else if (containerData.getSchemaVersion().equals(SCHEMA_V2)) {
+ putTxns(containerData, entry, newDeletionBlocks,
Review comment:
https://github.com/apache/ozone/pull/1702/files#diff-489818524a6cd8c72f568ade8c114d13436222ce4adb3a9c23a787f28c0b2fb0R118-R119
I think we can club multiple transactions for the same container into a single batch operation.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
Review comment:
This should be a common function for both the schemas and should be called for schema 1 as well.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -20,26 +20,24 @@
import java.io.File;
import java.io.IOException;
+import java.util.UUID;
import java.util.LinkedList;
+import java.util.Objects;
+import java.util.ArrayList;
import java.util.List;
import java.util.Map;
-import java.util.Objects;
-import java.util.UUID;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.scm.ScmConfigKeys;
import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException;
import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
-import org.apache.hadoop.hdds.utils.BackgroundService;
-import org.apache.hadoop.hdds.utils.BackgroundTask;
-import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
-import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
-import org.apache.hadoop.hdds.utils.db.BatchOperation;
-import org.apache.hadoop.hdds.utils.MetadataKeyFilters;
+import org.apache.hadoop.hdds.utils.*;
Review comment:
Star import.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java
##########
@@ -78,4 +97,9 @@ protected DatanodeSchemaTwoDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ public DBColumnFamilyDefinition<Long, DeletedBlocksTransaction>
+ getTxnBlocksColumnFamily() {
Review comment:
Let's rename it to getDeleteTransactionsColumnFamily.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
+ KeyValueContainerData containerData, DeletedBlocksTransaction delTX,
+ int newDeletionBlocks) throws IOException {
+ long containerId = delTX.getContainerID();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Processing Container : {}, DB path : {}", containerId,
+ containerData.getMetadataPath());
+ }
+
+ if (delTX.getTxID() < containerData.getDeleteTransactionId()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(String.format("Ignoring delete blocks for containerId: %d."
+ + " Outdated delete transactionId %d < %d", containerId,
+ delTX.getTxID(), containerData.getDeleteTransactionId()));
+ }
+ return;
+ }
Review comment:
This is a pre-processing step. I think this is also a common functionality for the schemas and should be called before any db operations. If containerTxnId is greater than the txnId to be processed we would ignore the transaction for both the schemas.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManagerHelper.java
##########
@@ -137,6 +143,29 @@ public TestStorageContainerManagerHelper(MiniOzoneCluster cluster,
return allBlocks;
}
+ public boolean verifyBlocksWithTxnTable(Map<Long, List<Long>> containerBlocks)
+ throws IOException {
+ Set<Long> containerIDs = containerBlocks.keySet();
+ for (Long entry : containerIDs) {
+ ReferenceCountedDB meta = getContainerMetadata(entry);
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ List<? extends Table.KeyValue<Long, DeletedBlocksTransaction>> kvs =
+ dnStoreTwoImpl.getTxnTable()
+ .getRangeKVs(null, Integer.MAX_VALUE, null);
+ List<Long> conID = new ArrayList<>();
Review comment:
Let's rename it to blocksInTxnTable?
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+
+ int deleteBlockCount = 0;
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ for (DeletedBlocksTransaction entry: delBlocks) {
Review comment:
Let's create a private function like deleteTransaction here. We also need to handle cases where only some blocks are deleted. Lets say the txn had 100 blocks and we successfully deleted 10 blocks then we remove only the 10 blocks from the db. Txn is removed only if all blocks are successfully deleted.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+
+ int deleteBlockCount = 0;
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ for (DeletedBlocksTransaction entry: delBlocks) {
+ for (Long blkLong : entry.getLocalIDList()) {
+ String blk = blkLong.toString();
+ BlockData blkInfo = blockDataTable.get(blk);
+ LOG.debug("Deleting block {}", blk);
+ try {
+ handler.deleteBlock(container, blkInfo);
+ deleteBlockCount++;
+ } catch (InvalidProtocolBufferException e) {
+ LOG.error("Failed to parse block info for block {}", blk, e);
+ } catch (IOException e) {
+ LOG.error("Failed to delete files for block {}", blk, e);
+ }
+ }
+ }
+
+ // Once blocks are deleted... remove the blockID from blockDataTable
+ // and also remove the transactions from txnTable.
+ try(BatchOperation batch = meta.getStore().getBatchHandler()
+ .initBatchOperation()) {
+ for (DeletedBlocksTransaction delTx : delBlocks) {
+ delTxTable.deleteWithBatch(batch, delTx.getTxID());
+ for (Long blk : delTx.getLocalIDList()) {
+ String bID = blk.toString();
+ meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
+ }
+ meta.getStore().getBatchHandler().commitBatchOperation(batch);
+ }
+ containerData.updateAndCommitDBCounters(meta, batch,
+ deleteBlockCount);
+ // update count of pending deletion blocks and block count in
+ // in-memory container status.
+ containerData.decrPendingDeletionBlocks(deleteBlockCount);
+ containerData.decrKeyCount(deleteBlockCount);
+ }
+
+ if (deleteBlockCount > 0) {
+ LOG.info("Container: {}, deleted blocks: {}, task elapsed time: {}ms",
+ containerData.getContainerID(), deleteBlockCount,
+ Time.monotonicNow() - startTime);
+ }
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
Review comment:
Also print exception.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+
+ int deleteBlockCount = 0;
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ for (DeletedBlocksTransaction entry: delBlocks) {
Review comment:
We can populate the batch operation in this function itself.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
Review comment:
Debug log should be enclosed in `if (LOG.isDebugEnabled())` check. Similarly for debug statement below.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
Review comment:
Let's rename it to deleteTxns.
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
+ for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ BlockID blockID =
+ ContainerTestHelper.getTestBlockID(containerID);
+ String deleteStateName = OzoneConsts.DELETING_KEY_PREFIX +
+ blockID.getLocalID();
+ BlockData kd = new BlockData(blockID);
+ List<ContainerProtos.ChunkInfo> chunks = Lists.newArrayList();
+ putChunksInBlock(numOfChunksPerBlock, j, chunks, buffer,
+ chunkManager, container, blockID);
+ kd.setChunks(chunks);
+ metadata.getStore().getBlockDataTable().put(
+ deleteStateName, kd);
+ container.getContainerData().incrPendingDeletionBlocks(1);
+ }
+ updateMetaData(data, container, numOfBlocksPerContainer,
+ numOfChunksPerBlock);
+ }
+ } else if (data.getSchemaVersion().equals(SCHEMA_V2)) {
+ Map<Long, List<Long>> containerBlocks = new HashMap<>();
+ int blockCount = 0;
+ for (int i = 0; i < numOfBlocksPerContainer; i++) {
+ txnID = txnID + 1;
BlockID blockID =
ContainerTestHelper.getTestBlockID(containerID);
- String deleteStateName = OzoneConsts.DELETING_KEY_PREFIX +
- blockID.getLocalID();
BlockData kd = new BlockData(blockID);
List<ContainerProtos.ChunkInfo> chunks = Lists.newArrayList();
- for (int k = 0; k < numOfChunksPerBlock; k++) {
- final String chunkName = String.format("block.%d.chunk.%d", j, k);
- final long offset = k * chunkLength;
- ContainerProtos.ChunkInfo info =
- ContainerProtos.ChunkInfo.newBuilder()
- .setChunkName(chunkName)
- .setLen(chunkLength)
- .setOffset(offset)
- .setChecksumData(Checksum.getNoChecksumDataProto())
- .build();
- chunks.add(info);
- ChunkInfo chunkInfo = new ChunkInfo(chunkName, offset, chunkLength);
- ChunkBuffer chunkData = buffer.duplicate(0, (int) chunkLength);
- chunkManager.writeChunk(container, blockID, chunkInfo, chunkData,
- WRITE_STAGE);
- chunkManager.writeChunk(container, blockID, chunkInfo, chunkData,
- COMMIT_STAGE);
- }
+ putChunksInBlock(numOfChunksPerBlock, i, chunks, buffer, chunkManager,
+ container, blockID);
kd.setChunks(chunks);
- metadata.getStore().getBlockDataTable().put(
- deleteStateName, kd);
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
+ String bID = blockID.getLocalID()+"";
+ metadata.getStore().getBlockDataTable().put(bID, kd);
+ }
container.getContainerData().incrPendingDeletionBlocks(1);
+
+ // In below if statements we are checking if a single container
+ // consists of more blocks than 'blockLimitPerTask' then we create
+ // (totalBlocksInContainer / blockLimitPerTask) transactions which
+ // consists of blocks equal to blockLimitPerTask and last transaction
+ // consists of blocks equal to
+ // (totalBlocksInContainer % blockLimitPerTask).
+ if (blockCount < blockLimitPerTask) {
+ if (containerBlocks.containsKey(containerID)) {
+ containerBlocks.get(containerID).add(blockID.getLocalID());
+ } else {
+ List<Long> item = new ArrayList<>();
+ item.add(blockID.getLocalID());
+ containerBlocks.put(containerID, item);
+ }
+ blockCount++;
+ }
+ boolean flag = false;
+ if (blockCount == blockLimitPerTask) {
+ createTxn(data, containerBlocks, txnID);
+ containerBlocks.clear();
+ blockCount = 0;
+ flag = true;
+ }
+ if (i == (numOfBlocksPerContainer - 1) && !flag) {
+ createTxn(data, containerBlocks, txnID);
+ }
Review comment:
```suggestion
containerBlocks.add(blockID.getLocalID());
blockCount++;
if (blockCount == blockLimitPerTask || i == (numOfBlocksPerContainer - 1)) {
createTxn(data, containerBlocks, txnID);
containerBlocks.clear();
blockCount = 0;
flag = true;
}
```
I think we can simplify the code here. We can use a List for storing containerBlocks.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java
##########
@@ -58,10 +61,26 @@
ChunkInfoList.class,
new ChunkInfoListCodec());
+ public static final DBColumnFamilyDefinition<Long, DeletedBlocksTransaction>
+ TXN =
Review comment:
Let's rename it to DELETE_TRANSACTION or sth better.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaTwoImpl.java
##########
@@ -30,6 +33,8 @@
*/
public class DatanodeStoreSchemaTwoImpl extends AbstractDatanodeStore {
+ private TypedTable<Long, DeletedBlocksTransaction> txnTable;
Review comment:
Let's rename it to deleteTransactionTable and similarly the getter can be renamed as well.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
Review comment:
exception needs to be printed.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547431325
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -129,7 +134,16 @@ public void handle(SCMCommand command, OzoneContainer container,
cont.getContainerData();
cont.writeLock();
try {
- deleteKeyValueContainerBlocks(containerData, entry);
+ if (containerData.getSchemaVersion().equals(SCHEMA_V1)) {
+ deleteKeyValueContainerBlocks(containerData, entry);
+ } else if (containerData.getSchemaVersion().equals(SCHEMA_V2)) {
+ putTxns(containerData, entry, newDeletionBlocks,
Review comment:
Thanks for pointing this out. I will raise a separate Jira for this issue.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548498899
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -242,52 +269,73 @@ private void deleteKeyValueContainerBlocks(
LOG.debug("Transited Block {} to DELETING state in container {}",
blk, containerId);
}
- } catch (IOException e) {
- // if some blocks failed to delete, we fail this TX,
- // without sending this ACK to SCM, SCM will resend the TX
- // with a certain number of retries.
- throw new IOException(
- "Failed to delete blocks for TXID = " + delTX.getTxID(), e);
- }
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Block {} not found or already under deletion in"
- + " container {}, skip deleting it.", blk, containerId);
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Block {} not found or already under deletion in"
+ + " container {}, skip deleting it.", blk, containerId);
+ }
}
}
+ updateMetaData(containerData, delTX, newDeletionBlocks, containerDB,
+ batch);
Review comment:
Thanks for pointing this out, moved commit operation after this updateMetaData function.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547485600
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
Review comment:
@errose28 Thanks for pointing this out. I have changed the code accordingly, now partially completed ContainerBackgroundTaskResult won’t be returned.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547487486
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaTwoImpl.java
##########
@@ -41,5 +46,13 @@ public DatanodeStoreSchemaTwoImpl(ConfigurationSource config,
throws IOException {
super(config, containerID, new DatanodeSchemaTwoDBDefinition(dbPath),
openReadOnly);
+ this.txnTable =
+ (TypedTable<Long, DeletedBlocksTransaction>) new
Review comment:
Changed.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547486433
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaTwoImpl.java
##########
@@ -41,5 +46,13 @@ public DatanodeStoreSchemaTwoImpl(ConfigurationSource config,
throws IOException {
super(config, containerID, new DatanodeSchemaTwoDBDefinition(dbPath),
openReadOnly);
+ this.txnTable =
+ (TypedTable<Long, DeletedBlocksTransaction>) new
+ DatanodeSchemaTwoDBDefinition(
+ dbPath).getTxnBlocksColumnFamily().getTable(getStore());
+ }
+
+ public TypedTable<Long, DeletedBlocksTransaction> getTxnTable(){
+ return txnTable;
Review comment:
Changed from TypedTable<> to Table<>.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547484942
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+
+ int deleteBlockCount = 0;
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ for (DeletedBlocksTransaction entry: delBlocks) {
Review comment:
Created the private function deleteTransaction().
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547483076
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
Review comment:
@lokeshj1703 Now updatemetadata() function is common for both schema 1 as well as schema 2.
@errose28 Now in a single batch operation we are performing both deletion and metadata update operations. Thanks for suggesting this.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547464323
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
+ KeyValueContainerData containerData, DeletedBlocksTransaction delTX,
+ int newDeletionBlocks) throws IOException {
+ long containerId = delTX.getContainerID();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Processing Container : {}, DB path : {}", containerId,
+ containerData.getMetadataPath());
+ }
+
+ if (delTX.getTxID() < containerData.getDeleteTransactionId()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(String.format("Ignoring delete blocks for containerId: %d."
+ + " Outdated delete transactionId %d < %d", containerId,
+ delTX.getTxID(), containerData.getDeleteTransactionId()));
+ }
+ return;
+ }
+ try(ReferenceCountedDB containerDB =
+ BlockUtils.getDB(containerData, conf)) {
+
+ if (newDeletionBlocks > 0) {
+ // Finally commit the DB counters.
+ try(BatchOperation batchOperation =
+ containerDB.getStore().getBatchHandler().initBatchOperation()) {
+ Table< String, Long > metadataTable = containerDB.getStore()
+ .getMetadataTable();
+
+ // In memory is updated only when existing delete transactionID is
+ // greater.
+ if (delTX.getTxID() > containerData.getDeleteTransactionId()) {
+ // Update in DB pending delete key count and delete transaction ID.
+ metadataTable.putWithBatch(batchOperation,
+ OzoneConsts.DELETE_TRANSACTION_KEY, delTX.getTxID());
+ }
+
+ long pendingDeleteBlocks =
+ containerData.getNumPendingDeletionBlocks() + newDeletionBlocks;
+ metadataTable.putWithBatch(batchOperation,
+ OzoneConsts.PENDING_DELETE_BLOCK_COUNT, pendingDeleteBlocks);
+ containerDB.getStore().getBatchHandler()
+ .commitBatchOperation(batchOperation);
Review comment:
As per my knowledge @errose28, SCM uses the same transaction on retries, and the transaction ID remains the same. Also, Lokesh's comment is addressed. Hence, I think '<' will work fine in this 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.
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] aryangupta1998 commented on pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#issuecomment-745334314
@errose28 Hi Ethan, can you review this PR?
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547483992
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java
##########
@@ -58,10 +61,26 @@
ChunkInfoList.class,
new ChunkInfoListCodec());
+ public static final DBColumnFamilyDefinition<Long, DeletedBlocksTransaction>
+ TXN =
Review comment:
Renamed it DELETE_TRANSACTION
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547487228
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java
##########
@@ -88,4 +88,10 @@ protected DatanodeSchemaOneDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ @Override
+ public DBColumnFamilyDefinition[] getColumnFamilies() {
+ return new DBColumnFamilyDefinition[] {getBlockDataColumnFamily(),
+ getMetadataColumnFamily(), getDeletedBlocksColumnFamily() };
Review comment:
Sorry, we don’t need to override. Removed.
----------------------------------------------------------------
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.
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 #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r544471398
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaTwoImpl.java
##########
@@ -41,5 +46,13 @@ public DatanodeStoreSchemaTwoImpl(ConfigurationSource config,
throws IOException {
super(config, containerID, new DatanodeSchemaTwoDBDefinition(dbPath),
openReadOnly);
+ this.txnTable =
+ (TypedTable<Long, DeletedBlocksTransaction>) new
+ DatanodeSchemaTwoDBDefinition(
+ dbPath).getTxnBlocksColumnFamily().getTable(getStore());
+ }
+
+ public TypedTable<Long, DeletedBlocksTransaction> getTxnTable(){
+ return txnTable;
Review comment:
All other table getters in `AbstractDatanodeStore` return `Table` type. I think we can do the same for this getter, making the cast in the constructor unnecessary.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -50,14 +48,22 @@
import org.apache.hadoop.ozone.container.common.utils.ReferenceCountedDB;
import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
import org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils;
+import org.apache.hadoop.ozone.container.metadata.DatanodeStore;
+import org.apache.hadoop.ozone.container.metadata.DatanodeStoreSchemaTwoImpl;
import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer;
import org.apache.hadoop.util.Time;
+import org.apache.hadoop.hdds.protocol.proto
+ .StorageContainerDatanodeProtocolProtos.DeletedBlocksTransaction;
import com.google.common.collect.Lists;
+
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_CONTAINER_LIMIT_PER_INTERVAL;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_CONTAINER_LIMIT_PER_INTERVAL_DEFAULT;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_LIMIT_PER_CONTAINER;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_LIMIT_PER_CONTAINER_DEFAULT;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V1;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V2;
+
Review comment:
Github won't let me comment on unchanged lines, but while we are touching this code might as well remove the old TODO on line 75. Pretty sure block deleting service works for all storage layouts now.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+
+ int deleteBlockCount = 0;
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ for (DeletedBlocksTransaction entry: delBlocks) {
+ for (Long blkLong : entry.getLocalIDList()) {
+ String blk = blkLong.toString();
+ BlockData blkInfo = blockDataTable.get(blk);
Review comment:
If it is not too difficult, could we add a test where a transaction is submitted for deletion multiple times to test this case? If such a test already exists and I missed it please point me to where it is.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
+ KeyValueContainerData containerData, DeletedBlocksTransaction delTX,
+ int newDeletionBlocks) throws IOException {
+ long containerId = delTX.getContainerID();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Processing Container : {}, DB path : {}", containerId,
+ containerData.getMetadataPath());
+ }
+
+ if (delTX.getTxID() < containerData.getDeleteTransactionId()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(String.format("Ignoring delete blocks for containerId: %d."
+ + " Outdated delete transactionId %d < %d", containerId,
+ delTX.getTxID(), containerData.getDeleteTransactionId()));
+ }
+ return;
+ }
+ try(ReferenceCountedDB containerDB =
+ BlockUtils.getDB(containerData, conf)) {
+
+ if (newDeletionBlocks > 0) {
+ // Finally commit the DB counters.
+ try(BatchOperation batchOperation =
+ containerDB.getStore().getBatchHandler().initBatchOperation()) {
+ Table< String, Long > metadataTable = containerDB.getStore()
+ .getMetadataTable();
+
+ // In memory is updated only when existing delete transactionID is
+ // greater.
+ if (delTX.getTxID() > containerData.getDeleteTransactionId()) {
+ // Update in DB pending delete key count and delete transaction ID.
+ metadataTable.putWithBatch(batchOperation,
+ OzoneConsts.DELETE_TRANSACTION_KEY, delTX.getTxID());
+ }
+
+ long pendingDeleteBlocks =
+ containerData.getNumPendingDeletionBlocks() + newDeletionBlocks;
+ metadataTable.putWithBatch(batchOperation,
+ OzoneConsts.PENDING_DELETE_BLOCK_COUNT, pendingDeleteBlocks);
+ containerDB.getStore().getBatchHandler()
+ .commitBatchOperation(batchOperation);
Review comment:
I'm not super familiar with the SCM code, but will SCM use the same transaction ID on retry if it sends a delete block commands to the datanode and doesn't get an ack? If so, this code will incorrectly update metadata in this case. Once [Lokesh's comment](https://github.com/apache/ozone/pull/1702/files#r544184639) is addressed, and if the `<` there is changed to `<=`, then I believe the check on 358 can be removed and we should be OK to handle duplicate transaction IDs.
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
Review comment:
For readability, can we refactor the contents of each schema 1 and schema 2 block of this if statement into their own methods, named according to which schema version they correspond to, as is done in other parts of the code? This way, there is a consistent pattern of calling either `*SchemaV1` or `*SchemaV2` when they need different code paths.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -129,7 +134,16 @@ public void handle(SCMCommand command, OzoneContainer container,
cont.getContainerData();
cont.writeLock();
try {
- deleteKeyValueContainerBlocks(containerData, entry);
+ if (containerData.getSchemaVersion().equals(SCHEMA_V1)) {
+ deleteKeyValueContainerBlocks(containerData, entry);
+ } else if (containerData.getSchemaVersion().equals(SCHEMA_V2)) {
+ putTxns(containerData, entry, newDeletionBlocks,
Review comment:
Can we rename `putTxns` and `deleteKeyValueContainerBlocks` to reflect the schema version they correspond to. For example, `markBlocksForDeletionSchemaV1` and `markBlocksForDeletionSchemaV2`?
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+
+ int deleteBlockCount = 0;
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ for (DeletedBlocksTransaction entry: delBlocks) {
+ for (Long blkLong : entry.getLocalIDList()) {
+ String blk = blkLong.toString();
+ BlockData blkInfo = blockDataTable.get(blk);
Review comment:
If a block has already been deleted and we try to delete it again like in [Lokesh's example](https://github.com/apache/ozone/pull/1702/files#r544211111), we should do nothing. This implementation will result in in a NullPointerException in `handler#deleteBlock` when the block is not found in the block data table. Note that the original version of the code handles this case when marking blocks for deletion with a prefix, but since we are operating on transactions, not prefixes, we have to do the check here when actually deleting the block.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -289,8 +291,7 @@ public void testBlockDeletionTransactions() throws Exception {
return false;
}
}, 1000, 10000);
- Assert.assertTrue(helper.getAllBlocks(containerIDs).isEmpty());
-
+ Assert.assertTrue(helper.verifyBlocksWithTxnTable(containerBlocks));
Review comment:
This will only work for schema version two. `SCHEMA_LATEST` should be able to be changed to V1 and this test should still pass. Therefore, we need something along the lines of `verifyWithSchemaV1` and `verifyWithSchemaV2` methods.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaTwoImpl.java
##########
@@ -41,5 +46,13 @@ public DatanodeStoreSchemaTwoImpl(ConfigurationSource config,
throws IOException {
super(config, containerID, new DatanodeSchemaTwoDBDefinition(dbPath),
openReadOnly);
+ this.txnTable =
+ (TypedTable<Long, DeletedBlocksTransaction>) new
Review comment:
The getters in the parent class return `Table`s instead of `TypedTable`s. I think we can do that here too, then we do not need this cast and the getter and private field can be changed accordingly.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java
##########
@@ -88,4 +88,10 @@ protected DatanodeSchemaOneDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ @Override
+ public DBColumnFamilyDefinition[] getColumnFamilies() {
+ return new DBColumnFamilyDefinition[] {getBlockDataColumnFamily(),
+ getMetadataColumnFamily(), getDeletedBlocksColumnFamily() };
Review comment:
Do we need this override? It looks the same as the parent method.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java
##########
@@ -88,4 +88,10 @@ protected DatanodeSchemaOneDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ @Override
+ public DBColumnFamilyDefinition[] getColumnFamilies() {
+ return new DBColumnFamilyDefinition[] {getBlockDataColumnFamily(),
+ getMetadataColumnFamily(), getDeletedBlocksColumnFamily() };
+ }
Review comment:
I don't think this override is necessary. It looks the same as the parent class method.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
+ KeyValueContainerData containerData, DeletedBlocksTransaction delTX,
+ int newDeletionBlocks) throws IOException {
+ long containerId = delTX.getContainerID();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Processing Container : {}, DB path : {}", containerId,
+ containerData.getMetadataPath());
+ }
+
+ if (delTX.getTxID() < containerData.getDeleteTransactionId()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(String.format("Ignoring delete blocks for containerId: %d."
+ + " Outdated delete transactionId %d < %d", containerId,
+ delTX.getTxID(), containerData.getDeleteTransactionId()));
+ }
+ return;
+ }
+ try(ReferenceCountedDB containerDB =
+ BlockUtils.getDB(containerData, conf)) {
+
+ if (newDeletionBlocks > 0) {
+ // Finally commit the DB counters.
+ try(BatchOperation batchOperation =
+ containerDB.getStore().getBatchHandler().initBatchOperation()) {
+ Table< String, Long > metadataTable = containerDB.getStore()
+ .getMetadataTable();
+
+ // In memory is updated only when existing delete transactionID is
+ // greater.
+ if (delTX.getTxID() > containerData.getDeleteTransactionId()) {
+ // Update in DB pending delete key count and delete transaction ID.
+ metadataTable.putWithBatch(batchOperation,
+ OzoneConsts.DELETE_TRANSACTION_KEY, delTX.getTxID());
+ }
+
+ long pendingDeleteBlocks =
+ containerData.getNumPendingDeletionBlocks() + newDeletionBlocks;
+ metadataTable.putWithBatch(batchOperation,
+ OzoneConsts.PENDING_DELETE_BLOCK_COUNT, pendingDeleteBlocks);
Review comment:
I am not very familiar with the SCM code, but will SCM use the same transaction ID to retry a deletion if it does not get an ack from the datanode? If so, this code will result in a duplicate metadata update if the DN did in fact get the first transaction. If IDs are reused for retries, then when [Lokesh's comment](https://github.com/apache/ozone/pull/1702/files#r544184639) is addressed, I think changing the `<` to `<=` in that check, then removing the check on line 358 should handle it.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
Review comment:
And if we do this for both versions, might as well batch writes of the transaction data and metadata together as well. Even though the original does not do this, there is a chance of a crash between the transaction update and the metadata update, causing them to become out of sync.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
Review comment:
Tagging on to this, can we update the metadata and deletion data in a single batch operation? This is not done in the original code, but it could lead to metadata becoming out of sync so I think it would be a good addition if we are calling the same metadata update method for both versions.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
Review comment:
Looks like in the earlier version of the code, an IOException here would have been propagated up out of the `call` method. Are there any pros or cons to catching the exception and returning the partially completed `ContainerBackgroundTaskResult` instead?
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
Review comment:
Not a huge issue, but technically we can violate the `blockLimitPerTask` here if `totalBlocks + delTx.getLocalIDList().size() > blockLimitPerTask` when we enter the last loop iteration. Probably better to do something like this:
```suggestion
while (iter.hasNext()) {
DeletedBlocksTransaction delTx = iter.next().getValue();
int txnBlockCount = delTx.getLocalIDList().size();
if (totalBlocks + txnBlockCount <= blockLimitPerTask) {
totalBlocks += txnBlockCount;
delBlocks.add(delTx);
}
}
```
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
Review comment:
It looks like the exception here was propagated by the `call` method in the original version of the code. Any pros/cons to returning the unfinished `ContainerBackgroundTaskReport` on exception 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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r551912601
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java
##########
@@ -88,4 +88,10 @@ protected DatanodeSchemaOneDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ @Override
+ public DBColumnFamilyDefinition[] getColumnFamilies() {
+ return new DBColumnFamilyDefinition[] {getBlockDataColumnFamily(),
+ getMetadataColumnFamily(), getDeletedBlocksColumnFamily() };
Review comment:
Yes as it’s in AbstractDatanodeDBDefinition therefore I think it should be here as well.
----------------------------------------------------------------
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.
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] lokeshj1703 commented on pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#issuecomment-755383363
Reverted the old commit and recommitted with Aryan as author.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r551909889
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
Review comment:
Done
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547484743
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
Review comment:
Renamed it deleteTxns.
----------------------------------------------------------------
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.
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] lokeshj1703 closed pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
lokeshj1703 closed pull request #1702:
URL: https://github.com/apache/ozone/pull/1702
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547484460
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaTwoImpl.java
##########
@@ -30,6 +33,8 @@
*/
public class DatanodeStoreSchemaTwoImpl extends AbstractDatanodeStore {
+ private TypedTable<Long, DeletedBlocksTransaction> txnTable;
Review comment:
Renamed it deleteTransactionTable.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547468585
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
Review comment:
Sorry @errose28, I am not able to get your point here. Can you explain a bit more about the kind of refactoring to be done here. 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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r551911821
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -289,8 +291,7 @@ public void testBlockDeletionTransactions() throws Exception {
return false;
}
}, 1000, 10000);
- Assert.assertTrue(helper.getAllBlocks(containerIDs).isEmpty());
-
+ Assert.assertTrue(helper.verifyBlocksWithTxnTable(containerBlocks));
Review comment:
Thanks, @errose28 for the suggestion, right now code will only use SCHEMA version 2. Right now I don't think we should use SCHEMA_LATEST to check schema versions as it will never use version 1. In follow-up Jira we will make changes as we did in TestBlockDeletingService so that it will run on both the version and then we can check the SCHEMA version as per your suggestion.
----------------------------------------------------------------
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.
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 #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r551391351
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java
##########
@@ -58,10 +61,26 @@
ChunkInfoList.class,
new ChunkInfoListCodec());
+ public static final DBColumnFamilyDefinition<Long, DeletedBlocksTransaction>
+ DELETE_TRANSACTION =
+ new DBColumnFamilyDefinition<>(
+ "txnTable",
Review comment:
Looks like when I did HDDS-3869 I used snake case on the column family names (`"block_data"`), even though the rest of the tables on other components uses camel case. Can you change the name on the block data table to be camel case (`"blockData"`) as well to match? Also maybe change this column family's name to `deleteTxns` for clarity, and since the other column families don't have `"table"` in their name?
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
Review comment:
Ah yes, I see that now. It seems SCM's `DeletedBlockLogImpl` takes a similar approach, where if the first transaction has more blocks than the block deleting limit, or the last transaction added puts the number of blocks over the limit, it allows it and it will be forwarded to the datanode. Although this isn't a super strict interpretation of the block deleting limit, it seems consistent with the rest of the code so I think it is ok.
##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
##########
@@ -158,65 +190,167 @@ private void createToDeleteBlocks(ContainerSet containerSet,
}
byte[] arr = randomAlphanumeric(1048576).getBytes(UTF_8);
ChunkBuffer buffer = ChunkBuffer.wrap(ByteBuffer.wrap(arr));
+ int txnID = 0;
for (int x = 0; x < numOfContainers; x++) {
long containerID = ContainerTestHelper.getTestContainerID();
KeyValueContainerData data =
new KeyValueContainerData(containerID, layout,
ContainerTestHelper.CONTAINER_MAX_SIZE,
UUID.randomUUID().toString(), datanodeUuid);
data.closeContainer();
+ data.setSchemaVersion(schemaVersion);
KeyValueContainer container = new KeyValueContainer(data, conf);
container.create(volumeSet,
new RoundRobinVolumeChoosingPolicy(), scmId);
containerSet.addContainer(container);
data = (KeyValueContainerData) containerSet.getContainer(
containerID).getContainerData();
- long chunkLength = 100;
- try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
- for (int j = 0; j < numOfBlocksPerContainer; j++) {
+ if (data.getSchemaVersion().equals(SCHEMA_V1)) {
+ try(ReferenceCountedDB metadata = BlockUtils.getDB(data, conf)) {
Review comment:
Sorry for the confusing explanation. I just meant to suggest that lines 210-226 be moved to a method called something like `createPendingDeleteBlocksSchema1` and lines 229-262 be moved to a method called `createPendingDeleteBlocksSchema2`.
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java
##########
@@ -88,4 +88,10 @@ protected DatanodeSchemaOneDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ @Override
+ public DBColumnFamilyDefinition[] getColumnFamilies() {
+ return new DBColumnFamilyDefinition[] {getBlockDataColumnFamily(),
+ getMetadataColumnFamily(), getDeletedBlocksColumnFamily() };
Review comment:
Looks like the method is still here, did you mean to delete it?
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java
##########
@@ -289,8 +291,7 @@ public void testBlockDeletionTransactions() throws Exception {
return false;
}
}, 1000, 10000);
- Assert.assertTrue(helper.getAllBlocks(containerIDs).isEmpty());
-
+ Assert.assertTrue(helper.verifyBlocksWithTxnTable(containerBlocks));
Review comment:
Do we really need a separate Jira for this? Looks like we could just check `OzoneConsts#SCHEMA_LATEST` to determine whether to call the new method or the old method here. Right now the code has a nice invariant where SCHEMA_LATEST can be set to schema version 1 or 2 and everything builds/all tests pass/system runs. I don't really think its worth violating this (even temporarily) since it requires just a few extra lines here, but let me know if supporting this for this test is more complicated and I have missed something.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547486172
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java
##########
@@ -88,4 +88,10 @@ protected DatanodeSchemaOneDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ @Override
+ public DBColumnFamilyDefinition[] getColumnFamilies() {
+ return new DBColumnFamilyDefinition[] {getBlockDataColumnFamily(),
+ getMetadataColumnFamily(), getDeletedBlocksColumnFamily() };
+ }
Review comment:
Removed.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547485043
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
Review comment:
Done
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +338,102 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
+ }
+ return crr;
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ delTxTable = dnStoreTwoImpl.getTxnTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getTxnTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+
+ int deleteBlockCount = 0;
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ for (DeletedBlocksTransaction entry: delBlocks) {
+ for (Long blkLong : entry.getLocalIDList()) {
+ String blk = blkLong.toString();
+ BlockData blkInfo = blockDataTable.get(blk);
+ LOG.debug("Deleting block {}", blk);
+ try {
+ handler.deleteBlock(container, blkInfo);
+ deleteBlockCount++;
+ } catch (InvalidProtocolBufferException e) {
+ LOG.error("Failed to parse block info for block {}", blk, e);
+ } catch (IOException e) {
+ LOG.error("Failed to delete files for block {}", blk, e);
+ }
+ }
+ }
+
+ // Once blocks are deleted... remove the blockID from blockDataTable
+ // and also remove the transactions from txnTable.
+ try(BatchOperation batch = meta.getStore().getBatchHandler()
+ .initBatchOperation()) {
+ for (DeletedBlocksTransaction delTx : delBlocks) {
+ delTxTable.deleteWithBatch(batch, delTx.getTxID());
+ for (Long blk : delTx.getLocalIDList()) {
+ String bID = blk.toString();
+ meta.getStore().getBlockDataTable().deleteWithBatch(batch, bID);
+ }
+ meta.getStore().getBatchHandler().commitBatchOperation(batch);
+ }
+ containerData.updateAndCommitDBCounters(meta, batch,
+ deleteBlockCount);
+ // update count of pending deletion blocks and block count in
+ // in-memory container status.
+ containerData.decrPendingDeletionBlocks(deleteBlockCount);
+ containerData.decrKeyCount(deleteBlockCount);
+ }
+
+ if (deleteBlockCount > 0) {
+ LOG.info("Container: {}, deleted blocks: {}, task elapsed time: {}ms",
+ containerData.getContainerID(), deleteBlockCount,
+ Time.monotonicNow() - startTime);
+ }
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID());
Review comment:
Done
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547484231
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java
##########
@@ -78,4 +97,9 @@ protected DatanodeSchemaTwoDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ public DBColumnFamilyDefinition<Long, DeletedBlocksTransaction>
+ getTxnBlocksColumnFamily() {
Review comment:
Rename it getDeleteTransactionsColumnFamily
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547483723
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java
##########
@@ -283,7 +319,58 @@ private void deleteKeyValueContainerBlocks(
// update pending deletion blocks count and delete transaction ID in
// in-memory container status
containerData.updateDeleteTransactionId(delTX.getTxID());
+ containerData.incrPendingDeletionBlocks(newDeletionBlocks);
+ }
+ }
+ }
+ }
+
+ private void updateMetaData(
+ KeyValueContainerData containerData, DeletedBlocksTransaction delTX,
+ int newDeletionBlocks) throws IOException {
+ long containerId = delTX.getContainerID();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Processing Container : {}, DB path : {}", containerId,
+ containerData.getMetadataPath());
+ }
+
+ if (delTX.getTxID() < containerData.getDeleteTransactionId()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(String.format("Ignoring delete blocks for containerId: %d."
+ + " Outdated delete transactionId %d < %d", containerId,
+ delTX.getTxID(), containerData.getDeleteTransactionId()));
+ }
+ return;
+ }
Review comment:
Made the required changes, now we are performing these pre-processing steps before DB operations.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547487674
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -50,14 +48,22 @@
import org.apache.hadoop.ozone.container.common.utils.ReferenceCountedDB;
import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
import org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils;
+import org.apache.hadoop.ozone.container.metadata.DatanodeStore;
+import org.apache.hadoop.ozone.container.metadata.DatanodeStoreSchemaTwoImpl;
import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer;
import org.apache.hadoop.util.Time;
+import org.apache.hadoop.hdds.protocol.proto
+ .StorageContainerDatanodeProtocolProtos.DeletedBlocksTransaction;
import com.google.common.collect.Lists;
+
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_CONTAINER_LIMIT_PER_INTERVAL;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_CONTAINER_LIMIT_PER_INTERVAL_DEFAULT;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_LIMIT_PER_CONTAINER;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_BLOCK_DELETING_LIMIT_PER_CONTAINER_DEFAULT;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V1;
+import static org.apache.hadoop.ozone.OzoneConsts.SCHEMA_V2;
+
Review comment:
Removed TODO.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r548499426
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingService.java
##########
@@ -312,9 +341,113 @@ public BackgroundTaskResult call() throws Exception {
}
crr.addAll(succeedBlocks);
return crr;
- } finally {
- container.writeUnlock();
+ } catch (IOException exception) {
+ LOG.warn(
+ "Deletion operation was not successful for container: " + container
+ .getContainerData().getContainerID(), exception);
+ throw exception;
+ }
+ }
+
+ public ContainerBackgroundTaskResult deleteViaSchema2(
+ ReferenceCountedDB meta, Container container, File dataDir,
+ long startTime) throws IOException {
+ ContainerBackgroundTaskResult crr = new ContainerBackgroundTaskResult();
+ try {
+ Table<String, BlockData> blockDataTable =
+ meta.getStore().getBlockDataTable();
+ DatanodeStore ds = meta.getStore();
+ DatanodeStoreSchemaTwoImpl dnStoreTwoImpl =
+ (DatanodeStoreSchemaTwoImpl) ds;
+ Table<Long, DeletedBlocksTransaction>
+ deleteTxns = dnStoreTwoImpl.getDeleteTransactionTable();
+ List<DeletedBlocksTransaction> delBlocks = new ArrayList<>();
+ int totalBlocks = 0;
+ try (TableIterator<Long,
+ ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
+ dnStoreTwoImpl.getDeleteTransactionTable().iterator()) {
+ while (iter.hasNext() && (totalBlocks < blockLimitPerTask)) {
+ DeletedBlocksTransaction delTx = iter.next().getValue();
+ totalBlocks += delTx.getLocalIDList().size();
+ delBlocks.add(delTx);
+ }
+ }
+
+ if (delBlocks.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No transaction found in container : {}",
+ containerData.getContainerID());
+ }
+ }
+
+ LOG.debug("Container : {}, To-Delete blocks : {}",
+ containerData.getContainerID(), delBlocks.size());
+ if (!dataDir.exists() || !dataDir.isDirectory()) {
+ LOG.error("Invalid container data dir {} : "
+ + "does not exist or not a directory", dataDir.getAbsolutePath());
+ return crr;
+ }
+
+ Handler handler = Objects.requireNonNull(ozoneContainer.getDispatcher()
+ .getHandler(container.getContainerType()));
+
+ int deleteBlockCount =
+ deleteTransaction(delBlocks, handler, blockDataTable, container);
Review comment:
Done.
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r551909474
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java
##########
@@ -58,10 +61,26 @@
ChunkInfoList.class,
new ChunkInfoListCodec());
+ public static final DBColumnFamilyDefinition<Long, DeletedBlocksTransaction>
+ DELETE_TRANSACTION =
+ new DBColumnFamilyDefinition<>(
+ "txnTable",
Review comment:
Done
----------------------------------------------------------------
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.
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] aryangupta1998 commented on a change in pull request #1702: HDDS-4369. Datanode should store the delete transaction as is in rocksDB
Posted by GitBox <gi...@apache.org>.
aryangupta1998 commented on a change in pull request #1702:
URL: https://github.com/apache/ozone/pull/1702#discussion_r547484231
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java
##########
@@ -78,4 +97,9 @@ protected DatanodeSchemaTwoDBDefinition(String dbPath) {
getDeletedBlocksColumnFamily() {
return DELETED_BLOCKS;
}
+
+ public DBColumnFamilyDefinition<Long, DeletedBlocksTransaction>
+ getTxnBlocksColumnFamily() {
Review comment:
Renamed it getDeleteTransactionsColumnFamily
----------------------------------------------------------------
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.
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