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 2022/08/15 04:02:38 UTC
[GitHub] [ozone] Xushaohong opened a new pull request, #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Xushaohong opened a new pull request, #3681:
URL: https://github.com/apache/ozone/pull/3681
## What changes were proposed in this pull request?
Currently, we find that the deleted block txns in SCM RDB are not updated anymore.
For the unhealthy container(no healthy replica and not enough replicas), SCM will not commit the txn and remove it from RDB until all desired replicas commit it. SCM will add the committed replica to the map `dnsWithTransactionCommitted` before all replicas commit the txn.
In the process of `getTransactions`, SCM will read the txn out from RDB and try to add it to the result, but if the txn existed in the `dnsWithTransactionCommitted`, SCM will ignore it. No matter whether ignoring the txn, the numBlocksDeletion is still counting this, which will lead to nonsense running, and actually returning empty txns when the cluster has a certain amount of unhealthy containers.
1. use the real num of blocks as the criterion
2. add the block.deletion.per-interval.max (originally it will be multiplied with the Replication Factor)
3. Fix UT and add the scenario of this case.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7125
## 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.
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
[GitHub] [ozone] Xushaohong commented on pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on PR #3681:
URL: https://github.com/apache/ozone/pull/3681#issuecomment-1219218657
@errose28 Hi Ethan, PTAL~
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by 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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #3681:
URL: https://github.com/apache/ozone/pull/3681#discussion_r954463173
##########
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?
The increase in default value is based on practice. In our cluster, the block deletion rate is quite not enough for the case users write and delete frequently and if the deletion speed is smaller than the write speed, the cluster space will be filled up some time.
For EC, the replica count could be 10 + 4, which leads to more blocks per file/key. While the deleting service is commonly used, we shall have an upgrade on this config. And 100,000 is a conservative value here.
> Also, can we update the config description to clarify this is operating on block replicas?
Sure
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #3681:
URL: https://github.com/apache/ozone/pull/3681#discussion_r954467940
##########
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:
The description comment is a good idea. I will update it.
> 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.
Just to clarify, originally the counter add a batch of blocks in a TXN each loop not only 1. Here actually is still the same, the TXN is the processing unit here. It stops at either the last TXN or the current TXN.
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #3681:
URL: https://github.com/apache/ozone/pull/3681#discussion_r954559340
##########
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:
It can be, but the refactor is meaningless.
Mine and the overload function you lined both calls the real commit function ```deletedBlockLog.commitTransactions()```, the difference is that :
The `DatanodeDeletedBlockTransactions` is more complex, the ```Map<UUID, List<DeletedBlocksTransaction>>``` could have different txns of `txID` for different `UUID`, the original overload only commit the same txns of `txID` for different `UUID`.
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #3681:
URL: https://github.com/apache/ozone/pull/3681#discussion_r954831202
##########
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?
Some cases related to committing certain num of txns also need the replication constant so I perserved it
> 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.
Done.
> Use the THREE replication constant as a multiple in the assert statements.
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #3681:
URL: https://github.com/apache/ozone/pull/3681#discussion_r954559340
##########
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:
It can be, but the refactor is meaningless.
Mine and the overload function you lined both calls the real commit function ```deletedBlockLog.commitTransactions()```, the difference is that :
The `DatanodeDeletedBlockTransactions` is more complex and replica-sensitive, the ```Map<UUID, List<DeletedBlocksTransaction>>``` could have different txns of `txID` for different `UUID`, the original overload only commit the same txns of `txID` for different `UUID`.
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by GitBox <gi...@apache.org>.
Xushaohong commented on code in PR #3681:
URL: https://github.com/apache/ozone/pull/3681#discussion_r954559340
##########
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:
It can be, but the refactor is meaningless.
Mine and the overload function you lined both calls the real commit function ```deletedBlockLog.commitTransactions()```, the difference is that :
The `DatanodeDeletedBlockTransactions` is more complex and datanode-sensitive, the ```Map<UUID, List<DeletedBlocksTransaction>>``` could have different txns of `txID` for different `UUID`, the original overload only commit the same txns of `txID` for different `UUID`.
--
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
[GitHub] [ozone] errose28 merged pull request #3681: HDDS-7125. Inaccurate numBlocksDeletion when getTransactions
Posted by GitBox <gi...@apache.org>.
errose28 merged PR #3681:
URL: https://github.com/apache/ozone/pull/3681
--
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