[GitHub] [ozone] errose28 commented on a diff in pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions

[GitBox <gi...@apache.org>]

errose28 commented on code in PR #3681:
URL: https://github.com/apache/ozone/pull/3681#discussion_r953106626


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -418,16 +418,15 @@ public DatanodeDeletedBlockTransactions getTransactions(
       try (TableIterator<Long,
           ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
                deletedBlockLogStateManager.getReadOnlyIterator()) {
-        int numBlocksAdded = 0;
         ArrayList<Long> txIDs = new ArrayList<>();
-        while (iter.hasNext() && numBlocksAdded < blockDeletionLimit) {
+        while (iter.hasNext() &&
+            transactions.getBlocksDeleted() < blockDeletionLimit) {

Review Comment:
   Can we add a comment here explaining that we are counting each replica as a unique block? It is not intuitive from this code block alone.
   
   Also worth noting that this is now a soft limit. If `transactions.getBlocksDeleted() == 9`, `blockDeletionLimit == 10`, and `txn` is to delete a block with 3 replicas, all 3 replicas will be queued for deletion before the loop exits, actually deleting 12 blocks. I don't think this is a serious problem but it is less intuitive to the casual reader vs. the original where the counter increased towards the limit by only 1 each time.



##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfig.java:
##########
@@ -79,7 +79,7 @@ public class ScmConfig {
 
   @Config(key = "block.deletion.per-interval.max",
       type = ConfigType.INT,
-      defaultValue = "20000",
+      defaultValue = "100000",

Review Comment:
   Why was the new value of 100,000 chosen? My understanding is that we used to be counting blocks based on ID, but now we are counting them based on replicas. Wouldn't this config's default value increase 3x to 60,000 for the same behavior?
   
   Also, can we update the config description to clarify this is operating on block replicas?



##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java:
##########
@@ -360,50 +371,78 @@ public void testResetCount() throws Exception {
     // Increment another time so it exceed the maxRetry.
     // On this call, count will be set to -1 which means TX eventually fails.
     incrementCount(txIDs);
-    blocks = getTransactions(40 * BLOCKS_PER_TXN);
+    blocks = getTransactions(40 * BLOCKS_PER_TXN * THREE);
     for (DeletedBlocksTransaction block : blocks) {
       Assertions.assertEquals(-1, block.getCount());
     }
 
     // If all TXs are failed, getTransactions call will always return nothing.
-    blocks = getTransactions(40 * BLOCKS_PER_TXN);
+    blocks = getTransactions(40 * BLOCKS_PER_TXN * THREE);
     Assertions.assertEquals(0, blocks.size());
 
     // Reset the retry count, these transactions should be accessible.
     resetCount(txIDs);
-    blocks = getTransactions(40 * BLOCKS_PER_TXN);
+    blocks = getTransactions(40 * BLOCKS_PER_TXN * THREE);
     for (DeletedBlocksTransaction block : blocks) {
       Assertions.assertEquals(0, block.getCount());
     }
-    Assertions.assertEquals(30, blocks.size());
+    Assertions.assertEquals(90, blocks.size());

Review Comment:
   It looks like most of these tests are not testing the throttling of the service so the number passed in to `getTransactions` is not relevant as long as it's sufficiently large, right? Could we
   - Pass Integer.MAX_VALUE to `getTransactions` when limiting the number of returned results is not important. Maybe a wrapper in the test like `getAllTransactions` which does this would be more intuitive.
   - Use the THREE replication constant as a multiple in the assert statements.
   
   There's also room to improve these tests in general by replacing the magic numbers with variables and asserting the number of transactions returned instead of commenting the expected behavior, but since this PR is not changing that part directly we don't have to make such a sweeping change.
   



##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java:
##########
@@ -265,6 +267,15 @@ private void commitTransactions(
         .collect(Collectors.toList()));
   }
 
+  private void commitTransactions(DatanodeDeletedBlockTransactions

Review Comment:
   Can this be refactored to call one of the existing overloads so that all `commitTransactions` calls end up at `commitTransactions(List<DeleteBlockTransactionResult> transactionResults, DatanodeDetails... dns)`. This makes the multiple overloads easier to follow.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org