HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one - #7532
Conversation
…ervice before retrying the same transaction again.
adoroszlai
changed the title
| try (TableIterator<Long, | ||
| ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> tmpIter = | ||
| deletedBlockLogStateManager.getReadOnlyIterator()) { | ||
| if (tmpIter.hasNext()) { | ||
| keyValue = tmpIter.next(); | ||
| if (keyValue.getKey() != firstProcessedTransactionKey) { | ||
| iter.seek(keyValue.getKey()); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| try (TableIterator<Long, | |
| ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> tmpIter = | |
| deletedBlockLogStateManager.getReadOnlyIterator()) { | |
| if (tmpIter.hasNext()) { | |
| keyValue = tmpIter.next(); | |
| if (keyValue.getKey() != firstProcessedTransactionKey) { | |
| iter.seek(keyValue.getKey()); | |
| } else { | |
| break; | |
| } | |
| } | |
| } | |
| iter.seekToFirst(); |
There was a problem hiding this comment.
@nandakumar131 Thanks for the suggestion, handled it.
|
There is some issue in latest change, looking into it. |
|
Fixed the issue, @nandakumar131 can you please help to relook. |
| ? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter = | ||
| deletedBlockLogStateManager.getReadOnlyIterator()) { | ||
| if (nextProcessTransactionId != -1) { | ||
| iter.seek(nextProcessTransactionId); |
There was a problem hiding this comment.
There is a possibility that the nextProcessTransactionId is deleted from the table. In that case comparing nextProcessTransactionId with keyValue.getKey() will make no sense.
| if (!iter.hasNext()) { | ||
| iter.seekToFirst(); | ||
| if (transactions.getBlocksDeleted() >= blockDeletionLimit) { | ||
| nextProcessTransactionId = -1; | ||
| } | ||
| } | ||
| } | ||
| if (iter.hasNext() && nextProcessTransactionId != -1) { | ||
| // Store this value so that next iteration starts from this element instead of beginning | ||
| nextProcessTransactionId = iter.next().getKey(); | ||
| } else { | ||
| // No more element in the table, start next iteration from the beginning | ||
| nextProcessTransactionId = -1L; |
There was a problem hiding this comment.
I can understand the idea here after spending some time going over the logic, but this is not easy to follow.
| if (nextProcessTransactionId == keyValue.getKey() && !firstTime) { | ||
| // Before already processed this transactionId in the current iteration. | ||
| // Table is completely iterated once | ||
| nextProcessTransactionId = -1; | ||
| break; |
There was a problem hiding this comment.
If the nextProcessTransactionId is removed form the table, we will not break here. This will end up adding duplicate entries into the transaction.
|
@ashishkumar50, thanks for working on this and also working on the review comments. |
|
@nandakumar131 Thanks for simplifying by skip processing the new lastTransactionId in some cases. Assume there are 15 transactions. |
I think above case is fine, as it will get next available transaction after 10th transaction to start next run which is fine. @nandakumar131, I have updated PR with your change. Can you please check. |
|
Thanks @ashishkumar50 for the contribution and thanks to @adoroszlai for the review. |
4 participants
What changes were proposed in this pull request?
In few cases when some transactions are problematic, that leads to block other transactions to be processed until it reaches max retry. To overcome this bottleneck we can give each transaction to get chance before same transaction being retried again.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11712
How was this patch tested?
New unit test