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 2021/01/15 08:03:44 UTC
[GitHub] [ozone] amaliujia opened a new pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
amaliujia opened a new pull request #1805:
URL: https://github.com/apache/ozone/pull/1805
## What changes were proposed in this pull request?
Details in the JIRA
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4708
## How was this patch tested?
UT
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r562370886
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java
##########
@@ -300,7 +347,8 @@ public void testRandomOperateTransactions() throws Exception {
// verify the number of added and committed.
try (TableIterator<Long,
? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
- scm.getScmMetadataStore().getDeletedBlocksTXTable().iterator()) {
+ scm.getScmMetadataStore().getDeletedBlocksTXTable()
Review comment:
Checkstyle complains this line exceeds 80 char limitation so I made this change.
----------------------------------------------------------------
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] amaliujia commented on pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-767148890
@lokeshj1703 PR rebased and conflicts solved. Any further comments?
----------------------------------------------------------------
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] amaliujia commented on pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-765101601
@lokeshj1703 you suggestion was actually great: simple and easy.
Now this PR changes less code than before.
----------------------------------------------------------------
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] bshashikant commented on pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-760768908
Probably, we should think about removing persisting the retry count in db altogether here.
cc ~ @lokeshj1703
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r562370643
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -387,6 +403,9 @@ public DatanodeDeletedBlockTransactions getTransactions(
}
}
purgeTransactions(txnsToBePurged);
+ for (DeletedBlocksTransaction trx : txnsToBePurged) {
+ transactionRetryCountMap.remove(trx.getTxID());
Review comment:
In fact this might be a bug: for purgeTransaction I didn't see `transactionToDNsCommitMap` is cleaned up properly.
Add the cleaning up of `transactionToDNsCommitMap` here.
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r562370718
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+ if (currentCount > -1) {
+ int nextCount = currentCount + 1;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount / 100) - (currentCount / 100) > 0) {
Review comment:
`nextCount % 100 == 0` is good.
----------------------------------------------------------------
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] amaliujia commented on pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-761615674
The writing the retry count into DB will still be useful at least when the retry count exceed the maxRetry. It is useful when some blocks cannot be deleted by a reason thus there is a record in DB such that people can analyze the reason.
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r559824378
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,33 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount = -1;
+ if (transactionRetryCountMap.containsKey(txID)) {
+ currentCount = transactionRetryCountMap.get(txID);
+ } else {
+ currentCount = block.getCount();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+
+ if (currentCount > -1) {
+ int nextCount = currentCount++;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount % 100) - (currentCount % 100) > 0) {
Review comment:
O I know what you were suggesting:
It needs to be at least `(nextCount / 100) - (currentCount / 100)` :)
----------------------------------------------------------------
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 #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-768099567
@amaliujia Thanks for the contribution! 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] lokeshj1703 commented on a change in pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r559528037
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,33 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount = -1;
+ if (transactionRetryCountMap.containsKey(txID)) {
+ currentCount = transactionRetryCountMap.get(txID);
+ } else {
+ currentCount = block.getCount();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+
+ if (currentCount > -1) {
+ int nextCount = currentCount++;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount % 100) - (currentCount % 100) > 0) {
Review comment:
`(nextCount % 100) - (currentCount % 100)` This would always be 0 since currentCount now would be equal to nextCount. I think we can also use sth like `nextCount % 100 == 99`?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -92,6 +94,7 @@ public DeletedBlockLogImpl(ConfigurationSource conf,
// maps transaction to dns which have committed it.
transactionToDNsCommitMap = new ConcurrentHashMap<>();
+ transactionRetryCountMap = new ConcurrentHashMap<>();
Review comment:
We would need to remove entries from this map in functions `commitTransactions` and `purgeTransactions`.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,33 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount = -1;
+ if (transactionRetryCountMap.containsKey(txID)) {
+ currentCount = transactionRetryCountMap.get(txID);
+ } else {
+ currentCount = block.getCount();
Review comment:
```suggestion
int currentCount = transactionRetryCountMap.getOrDefault(txID, block.getCount());
```
We can use the getOrDefault api here.
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java
##########
@@ -238,6 +238,63 @@ public void testIncrementCount() throws Exception {
Assert.assertEquals(blocks.size(), 0);
}
+ @Test
+ public void testIncrementCountLessFrequentWritingToDB() throws Exception {
+ int maxRetry = conf.getInt(OZONE_SCM_BLOCK_DELETION_MAX_RETRY, 120);
+
+ // Create 1 TXs in the log.
+ for (Map.Entry<Long, List<Long>> entry : generateData(1).entrySet()){
+ deletedBlockLog.addTransaction(entry.getKey(), entry.getValue());
+ }
+
+ // This will return all TXs, total num 30.
+ List<DeletedBlocksTransaction> blocks =
+ getTransactions(40 * BLOCKS_PER_TXN);
+ List<Long> txIDs = blocks.stream().map(DeletedBlocksTransaction::getTxID)
+ .collect(Collectors.toList());
+ Assert.assertEquals(txIDs.size(), 1);
+ long txId = txIDs.get(0);
+ for (int i = 0; i < 50; i++) {
+ deletedBlockLog.incrementCount(txIDs);
+ }
+ for (DeletedBlocksTransaction block : blocks) {
+ // block count should not be updated.
+ Assert.assertEquals(0, block.getCount());
+ }
+
+ for (int i = 0; i < 60; i++) {
+ deletedBlockLog.incrementCount(txIDs);
+ }
+ for (DeletedBlocksTransaction block : blocks) {
+ // block count should be updated to 100.
+ Assert.assertEquals(100, block.getCount());
+ }
+
+ for (int i = 0; i < 50; i++) {
+ deletedBlockLog.incrementCount(txIDs);
+ }
+ for (DeletedBlocksTransaction block : blocks) {
+ // block count should be updated to -1 as retry count exceeds maxRetry
+ Assert.assertEquals(-1, block.getCount());
+ }
+
+// for (int i = 0; i < maxRetry; i++) {
Review comment:
We can remove the commented code here?
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r562370643
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -387,6 +403,9 @@ public DatanodeDeletedBlockTransactions getTransactions(
}
}
purgeTransactions(txnsToBePurged);
+ for (DeletedBlocksTransaction trx : txnsToBePurged) {
+ transactionRetryCountMap.remove(trx.getTxID());
Review comment:
In fact this might be a bug: for purgeTransaction I didn't `transactionToDNsCommitMap` is cleaned up properly.
Add the cleaning up of `transactionToDNsCommitMap` here.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+ if (currentCount > -1) {
+ int nextCount = currentCount + 1;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount / 100) - (currentCount / 100) > 0) {
Review comment:
`nextCount % 100 == 0` is good.
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java
##########
@@ -300,7 +347,8 @@ public void testRandomOperateTransactions() throws Exception {
// verify the number of added and committed.
try (TableIterator<Long,
? extends Table.KeyValue<Long, DeletedBlocksTransaction>> iter =
- scm.getScmMetadataStore().getDeletedBlocksTXTable().iterator()) {
+ scm.getScmMetadataStore().getDeletedBlocksTXTable()
Review comment:
Checkstyle complains this line exceeds 80 char limitation so I made this change.
----------------------------------------------------------------
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] amaliujia commented on pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-767148890
@lokeshj1703 PR rebased and conflicts solved. Any further comments?
----------------------------------------------------------------
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] amaliujia commented on pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-765101601
@lokeshj1703 you suggestion was actually great: simple and easy.
Now this PR changes less code than before.
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r559817790
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,33 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount = -1;
+ if (transactionRetryCountMap.containsKey(txID)) {
+ currentCount = transactionRetryCountMap.get(txID);
+ } else {
+ currentCount = block.getCount();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+
+ if (currentCount > -1) {
+ int nextCount = currentCount++;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount % 100) - (currentCount % 100) > 0) {
Review comment:
O I see at line 154 I did `int nextCount = currentCount++;`.
Indeed it means nextCount = currentCount. (use currentCount then ++).
I updated to `int nextCount = currentCount + 1;` and now `nextCount % 100) - (currentCount % 100)` is supposed to work
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r562370643
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -387,6 +403,9 @@ public DatanodeDeletedBlockTransactions getTransactions(
}
}
purgeTransactions(txnsToBePurged);
+ for (DeletedBlocksTransaction trx : txnsToBePurged) {
+ transactionRetryCountMap.remove(trx.getTxID());
Review comment:
In fact this might be a bug: for purgeTransaction I didn't `transactionToDNsCommitMap` is cleaned up properly.
Add the cleaning up of `transactionToDNsCommitMap` here.
----------------------------------------------------------------
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 #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-762244350
> Probably, we should think about removing persisting the retry count in db altogether here.
Sure @bshashikant . Let's discuss this in a separate jira?
----------------------------------------------------------------
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 #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r561739335
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
Review comment:
Redundant change.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+ if (currentCount > -1) {
+ int nextCount = currentCount + 1;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount / 100) - (currentCount / 100) > 0) {
Review comment:
It's difficult to understand the logic here. Can we replace it using %? Perhaps `nextCount % 100 == 0 or 99`?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -387,6 +403,9 @@ public DatanodeDeletedBlockTransactions getTransactions(
}
}
purgeTransactions(txnsToBePurged);
+ for (DeletedBlocksTransaction trx : txnsToBePurged) {
+ transactionRetryCountMap.remove(trx.getTxID());
Review comment:
Unrelated to PR: Can we also remove from transactionToDNsCommitMap here for better surity?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+ if (currentCount > -1) {
+ int nextCount = currentCount + 1;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount / 100) - (currentCount / 100) > 0) {
+ // write retry count after every 100 retries into DB.
+ builder.setCount(nextCount);
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ }
+ transactionRetryCountMap.put(txID, nextCount);
Review comment:
Can be moved inside else if
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+ if (currentCount > -1) {
+ int nextCount = currentCount + 1;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
Review comment:
```suggestion
transactionRetryCountMap.remove(txID);
```
We can remove the entry from map here.
----------------------------------------------------------------
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 #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#discussion_r561739335
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
Review comment:
Redundant change.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+ if (currentCount > -1) {
+ int nextCount = currentCount + 1;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount / 100) - (currentCount / 100) > 0) {
Review comment:
It's difficult to understand the logic here. Can we replace it using %? Perhaps `nextCount % 100 == 0 or 99`?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -387,6 +403,9 @@ public DatanodeDeletedBlockTransactions getTransactions(
}
}
purgeTransactions(txnsToBePurged);
+ for (DeletedBlocksTransaction trx : txnsToBePurged) {
+ transactionRetryCountMap.remove(trx.getTxID());
Review comment:
Unrelated to PR: Can we also remove from transactionToDNsCommitMap here for better surity?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+ if (currentCount > -1) {
+ int nextCount = currentCount + 1;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ } else if ((nextCount / 100) - (currentCount / 100) > 0) {
+ // write retry count after every 100 retries into DB.
+ builder.setCount(nextCount);
+ scmMetadataStore.getDeletedBlocksTXTable().put(txID,
+ builder.build());
+ }
+ transactionRetryCountMap.put(txID, nextCount);
Review comment:
Can be moved inside else if
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java
##########
@@ -139,19 +142,31 @@ public void incrementCount(List<Long> txIDs) throws IOException {
}
continue;
}
- DeletedBlocksTransaction.Builder builder = block.toBuilder();
- int currentCount = block.getCount();
- if (currentCount > -1) {
- builder.setCount(++currentCount);
+
+ int currentCount =
+ transactionRetryCountMap.getOrDefault(txID, block.getCount());
+ if (currentCount >= 99) {
+ System.out.println();
}
- // if the retry time exceeds the maxRetry value
- // then set the retry value to -1, stop retrying, admins can
- // analyze those blocks and purge them manually by SCMCli.
- if (currentCount > maxRetry) {
- builder.setCount(-1);
+ if (currentCount > -1) {
+ int nextCount = currentCount + 1;
+ DeletedBlocksTransaction.Builder builder = block.toBuilder();
+ if (nextCount > maxRetry) {
+ // if the retry time exceeds the maxRetry value
+ // then set the retry value to -1, stop retrying, admins can
+ // analyze those blocks and purge them manually by SCMCli.
+ builder.setCount(-1);
+ nextCount = -1;
Review comment:
```suggestion
transactionRetryCountMap.remove(txID);
```
We can remove the entry from map here.
----------------------------------------------------------------
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 #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
lokeshj1703 closed pull request #1805:
URL: https://github.com/apache/ozone/pull/1805
----------------------------------------------------------------
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] amaliujia commented on pull request #1805: HDDS-4708. Optimization: update RetryCount less frequently (update once per ~100)
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #1805:
URL: https://github.com/apache/ozone/pull/1805#issuecomment-762512583
@lokeshj1703 comments addressed.
Had to refactor testing code a bit to fix the failed UT.
----------------------------------------------------------------
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