[GitHub] [ozone] sodonnel commented on pull request #3799: HDDS-7287. Send deleteBlocksRequest with correct retry count

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

sodonnel commented on PR #3799:
URL: https://github.com/apache/ozone/pull/3799#issuecomment-1271414316

   I think this change makes sense and I think the change you made to the unit test validates the first scenario:
   
   > but the DB value won't be changed until exceeding the max retry times
   
   I'm not sure any tests catch the second scenario though:
   
   > Reset retry count didn't reset the value of transactionToRetryCountMap.
   
   Could you add or modify a test to correctly validate that the reset call correctly resets the transactionToRetryCountMap? Perhaps by incrementing failed, resetting, and incrementing again to some other value?
   
   TestResetCount seems to check that the DB is cleared when reset is called, but it doesn't increment again, which think would cause the bug, as the map would hold the old value and increment it incorrectly. This is the TestRestCount method:
   
   ```
   // Reset the retry count, these transactions should be accessible.
       resetCount(txIDs);
       blocks = getAllTransactions();
       for (DeletedBlocksTransaction block : blocks) {
         Assertions.assertEquals(0, block.getCount());
       }
       Assertions.assertEquals(30 * THREE, blocks.size());
   ```
   
   I think we need something to increment before this part which should increment to "1" but before this fix it will increment to whatever was stored in the map.


-- 
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