You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ashishkumar50 (via GitHub)" <gi...@apache.org> on 2024/12/05 08:11:01 UTC
[PR] HDDS-11712. Iterate whole scm delete block table in SCMBlockDeletingService before retrying the same transaction again. [ozone]
ashishkumar50 opened a new pull request, #7532:
URL: https://github.com/apache/ozone/pull/7532
## 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
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on PR #7532:
URL: https://github.com/apache/ozone/pull/7532#issuecomment-2538197582
There is some issue in latest change, looking into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #7532:
URL: https://github.com/apache/ozone/pull/7532#discussion_r1884745388
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -344,6 +345,39 @@ public DatanodeDeletedBlockTransactions getTransactions(
try (TableIterator<Long,
? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
deletedBlockLogStateManager.getReadOnlyIterator()) {
+
+ if (lastProcessedTransactionId == 13) {
+ lastProcessedTransactionId = 12;
+ }
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.
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #7532:
URL: https://github.com/apache/ozone/pull/7532#discussion_r1881369952
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -360,6 +366,12 @@ public DatanodeDeletedBlockTransactions getTransactions(
transactions.getBlocksDeleted() < blockDeletionLimit) {
Table.KeyValue<Long, DeletedBlocksTransaction> keyValue = iter.next();
DeletedBlocksTransaction txn = keyValue.getValue();
+ if (firstProcessedTransactionKey == keyValue.getKey()) {
+ break;
+ }
+ if (firstProcessedTransactionKey == -1) {
+ firstProcessedTransactionKey = keyValue.getKey();
+ }
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.
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #7532:
URL: https://github.com/apache/ozone/pull/7532#discussion_r1883858048
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -344,6 +345,39 @@ public DatanodeDeletedBlockTransactions getTransactions(
try (TableIterator<Long,
? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
deletedBlockLogStateManager.getReadOnlyIterator()) {
+
+ if (lastProcessedTransactionId == 13) {
+ lastProcessedTransactionId = 12;
+ }
Review Comment:
Is this change committed by mistake?
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 merged PR #7532:
URL: https://github.com/apache/ozone/pull/7532
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on PR #7532:
URL: https://github.com/apache/ozone/pull/7532#issuecomment-2540862453
@nandakumar131 Thanks for simplifying by skip processing the new lastTransactionId in some cases.
One thing i think needs to corrected:
This is not setting correctly in below case
`lastProcessedTransactionId = keyValue != null ? keyValue.getKey() : -1;`
Assume there are 15 transactions.
And SCM returns maximum 10 transactions in one run.
In this case transaction 1-10 is returned back and lastProcessedTransactionId is set it as 10
In second run, again lastProcessedTransactionId is set it as 10 instead of 15.
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on PR #7532:
URL: https://github.com/apache/ozone/pull/7532#issuecomment-2541131674
> @nandakumar131 Thanks for simplifying by skip processing the new lastTransactionId in some cases. One thing i think needs to corrected: This is not setting correctly in below case `lastProcessedTransactionId = keyValue != null ? keyValue.getKey() : -1;`
>
> Assume there are 15 transactions. And SCM returns maximum 10 transactions in one run. In this case transaction 1-10 is returned back and lastProcessedTransactionId is set it as 10 In second run, again lastProcessedTransactionId is set it as 10. I think it should set it as -1?
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.
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #7532:
URL: https://github.com/apache/ozone/pull/7532#issuecomment-2540763918
@ashishkumar50, thanks for working on this and also working on the review comments.
I tried to simplify the logic and make it easy to understand, please take a look at this - https://github.com/apache/ozone/commit/0debb781a3088c07b35aae4828cb20a226079f38
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #7532:
URL: https://github.com/apache/ozone/pull/7532#discussion_r1883860842
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -344,6 +345,39 @@ public DatanodeDeletedBlockTransactions getTransactions(
try (TableIterator<Long,
? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
deletedBlockLogStateManager.getReadOnlyIterator()) {
+
+ if (lastProcessedTransactionId == 13) {
+ lastProcessedTransactionId = 12;
+ }
Review Comment:
Oh yeah I was testing, will remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #7532:
URL: https://github.com/apache/ozone/pull/7532#discussion_r1877927395
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -360,6 +366,12 @@ public DatanodeDeletedBlockTransactions getTransactions(
transactions.getBlocksDeleted() < blockDeletionLimit) {
Table.KeyValue<Long, DeletedBlocksTransaction> keyValue = iter.next();
DeletedBlocksTransaction txn = keyValue.getValue();
+ if (firstProcessedTransactionKey == keyValue.getKey()) {
+ break;
+ }
+ if (firstProcessedTransactionKey == -1) {
+ firstProcessedTransactionKey = keyValue.getKey();
+ }
Review Comment:
We can reuse the `lastProcessedTransactionId` instead of introducing a local `firstProcessedTransactionKey` variable.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -386,6 +398,25 @@ public DatanodeDeletedBlockTransactions getTransactions(
LOG.warn("Container: {} was not found for the transaction: {}.", id, txn);
txIDs.add(txn.getTxID());
}
+ if (!iter.hasNext()) {
+ 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;
+ }
+ }
+ }
Review Comment:
```suggestion
iter.seekToFirst();
```
--
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
Re: [PR] HDDS-11712. Iterate whole scm delete block table in SCMBlockDeletingService before retrying the same transaction again. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7532:
URL: https://github.com/apache/ozone/pull/7532#discussion_r1870954269
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java:
##########
@@ -55,6 +55,7 @@
import org.apache.hadoop.ozone.protocol.commands.DeleteBlocksCommand;
import org.apache.hadoop.ozone.protocol.commands.SCMCommand;
import org.junit.jupiter.api.AfterEach;
+import static org.assertj.core.api.Assertions.assertThat;
Review Comment:
nit: please don't move around imports unnecessarily
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #7532:
URL: https://github.com/apache/ozone/pull/7532#discussion_r1881370128
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -386,6 +398,25 @@ public DatanodeDeletedBlockTransactions getTransactions(
LOG.warn("Container: {} was not found for the transaction: {}.", id, txn);
txIDs.add(txn.getTxID());
}
+ if (!iter.hasNext()) {
+ 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;
+ }
+ }
+ }
Review Comment:
@nandakumar131 Thanks for the suggestion, handled it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on PR #7532:
URL: https://github.com/apache/ozone/pull/7532#issuecomment-2539268619
Fixed the issue, @nandakumar131 can you please help to relook.
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #7532:
URL: https://github.com/apache/ozone/pull/7532#issuecomment-2547651114
Thanks @ashishkumar50 for the contribution and thanks to @adoroszlai for the review.
--
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
Re: [PR] HDDS-11712. Process other DeletedBlocksTransaction before retrying failed one [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #7532:
URL: https://github.com/apache/ozone/pull/7532#discussion_r1883435451
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -344,6 +346,9 @@ public DatanodeDeletedBlockTransactions getTransactions(
try (TableIterator<Long,
? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
deletedBlockLogStateManager.getReadOnlyIterator()) {
+ if (nextProcessTransactionId != -1) {
+ iter.seek(nextProcessTransactionId);
Review 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.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -386,6 +406,19 @@ public DatanodeDeletedBlockTransactions getTransactions(
LOG.warn("Container: {} was not found for the transaction: {}.", id, txn);
txIDs.add(txn.getTxID());
}
+ 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;
Review Comment:
I can understand the idea here after spending some time going over the logic, but this is not easy to follow.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java:
##########
@@ -360,6 +366,17 @@ public DatanodeDeletedBlockTransactions getTransactions(
transactions.getBlocksDeleted() < blockDeletionLimit) {
Table.KeyValue<Long, DeletedBlocksTransaction> keyValue = iter.next();
DeletedBlocksTransaction txn = keyValue.getValue();
+
+ if (nextProcessTransactionId == keyValue.getKey() && !firstTime) {
+ // Before already processed this transactionId in the current iteration.
+ // Table is completely iterated once
+ nextProcessTransactionId = -1;
+ break;
Review Comment:
If the `nextProcessTransactionId` is removed form the table, we will not break here. This will end up adding duplicate entries into the 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.
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