HDDS-15990. Remove setExclusiveManualCompaction from CompactionService

[ivandika3]

What changes were proposed in this pull request?

We should remove setExclusiveManualCompaction so that automatic compactions can still compact L0 -> L1 files and prevent write stalls.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15990

How was this patch tested?

CI.

https://github.com/ivandika3/ozone/actions/runs/30231577577

[ashishkumar50]

@ivandika3 Thanks for the patch, whether you are using kSkip or kForce for the compaction?
As in below PR now compaction is using kSkip by default which will not compact bottommost layer and should be fast enough.
#10419

[ivandika3]

Thanks @ashishkumar50 for checking this patch.

Thanks for the patch, whether you are using kSkip or kForce for the compaction?
As in below PR now compaction is using kSkip by default which will not compact bottommost layer and should be fast enough.

We are using kForce, #10419 is a very recent patch so we are not using this.

However, I don't see any good reason why we have to setExclusiveManualCompaction rather than just prematurely preventing hypothetical overhead of concurrent compactions (which I don't think is well understood by the Ozone community). For example, we already have a previous DatanodeStoreSchemaThreeImpl#compactionIfNeeded in datanode which does not setExclusiveManualCompaction and we have no (known) write stall issues (we still need to check this). So unless there is a stress test proving that the concurrent manual and automatic compaction actually has any meaningful overhead, we should remove it since now we know that there is a very real production risk (not just hypotheticals).

From my understanding the kForce is the entire point of the compaction service. If there are a lot of tombstones in the bottommost layer, we want to clean them up. Additionally, is the difference between kSkip and automatic compaction well-understood? Also the chance of tombstones in the bottommost level.

should be fast enough

If the non-bottommost levels are very big, the issue might still happen. Unless we have a strong guarantee or data that ensures that there will be no write stall, IMO this exclusive compaction adds another unnecessary surprise in OM stability.

[ashishkumar50]

However, I don't see any good reason why we have to setExclusiveManualCompaction rather than just prematurely preventing hypothetical overhead of concurrent compactions (which I don't think is well understood by the Ozone community). For example, we already have a previous DatanodeStoreSchemaThreeImpl#compactionIfNeeded in datanode which does not setExclusiveManualCompaction and we have no (known) write stall issues (we still need to check this). So unless there is a stress test proving that the concurrent manual and automatic compaction actually has any meaningful overhead, we should remove it since now we know that there is a very real production risk (not just hypotheticals).

@jojochuang Do you have any thought on this, why setExclusiveManualCompaction was done?

[jojochuang]

I don't really recall. Though I'd suggest to keep it simple rather than some kind of heuristics to predict whether to add this flag or not. With the recent RocksDB update (from 7 -> 10: #9813) a number of built-in compaction kicks in automatically. Probably a good idea to revisit the built-in mechanisms rather than reinvent the wheels.

E.g. https://issues.apache.org/jira/browse/HDDS-12254

[jojochuang]

@ivandika3 this is great writeup. Has this been tested in your prod environment? If so let's merge it asap.

[swamirishi]

@ivandika3 Thank you for the patch. IMO we don't have the need of having CompactionService in OM. Om rocksdb has problems with the way we are using rocksdb. On a long term basis I would suggest we actually merge these prs and get rid of background compaction service altogether and let rocksdbs background service take care of compactions:
#9560
#9553
#9423
We won't have the tombstone and slow rocksdb iterator problem to begin with.

[ivandika3]

Probably a good idea to revisit the built-in mechanisms rather than reinvent the wheels.

+1, need to revisit RocksDB as a whole.

this is great writeup. Has this been tested in your prod environment? If so let's merge it asap.

Not yet since we need to reproduce it in our test cluster. We have previously run some stress test in our test cluster to check the risk of the compaction service, but the issue is not triggered during the test so not sure how hard it is to reproduce it in our test cluster.

Currently, we decided to disable compaction service for the foreseeable future until we understand the RocksDB enough to be confident to enable it, the risk is too high.

We won't have the tombstone and slow rocksdb iterator problem to begin with.

@swamirishi Seems your patches are related to deleteRange and snapshot. Our cluster does encounter tombstone and slow rocksdb iterator caused by holding the RocksDB iterator during RocksDB seek in the LEGACY bucket , see #9932 . Not sure how the attached patches will help prevent our case (we don't use Ozone snapshot in production).

[swamirishi]

Probably a good idea to revisit the built-in mechanisms rather than reinvent the wheels.

+1, need to revisit RocksDB as a whole.

this is great writeup. Has this been tested in your prod environment? If so let's merge it asap.

Not yet since we need to reproduce it in our test cluster. We have previously run some stress test in our test cluster to check the risk of the compaction service, but the issue is not triggered during the test so not sure how hard it is to reproduce it in our test cluster.

Currently, we decided to disable compaction service for the foreseeable future until we understand the RocksDB enough to be confident to enable it, the risk is too high.

We won't have the tombstone and slow rocksdb iterator problem to begin with.

@swamirishi Seems your patches are related to deleteRange and snapshot. Our cluster does encounter tombstone and slow rocksdb iterator caused by holding the RocksDB iterator during RocksDB seek in the LEGACY bucket , see #9932 . Not sure how the attached patches will help prevent our case (we don't use Ozone snapshot in production).

I am sorry @ivandika3 I redundantly added links to the same patches twice and forgot to add this. The following PR solves slow iterator problem for FSO. For OBS I would love to understand how the keys are being deleted in production. Is there some bulk delete in the buckets? We can do something similar for the delete as well and add delete range tombstone as well. BTW can you check what is the size of each OMKeyInfo in your keyTable/FileTable/DirectoryTable. Do you guys have ACL bloat problem?

[ivandika3]

Is there some bulk delete in the buckets?

@swamirishi Yes, we have the S3 lifecycle configuration service (HDDS-8342) that generates a few millions deletions in a very short time. In LEGACY bucket getFileStatus can trigger a RocksDB seek that is done under a bucket lock, if there are a lot of tombstones, then the lock will be held for a long time which can block Ratis applier and cause the OM write to be stuck. We have resolved it in #9932 by moving the createFakeDirIfShould outside the lock, but new patches can easily fall to the same trap since some people might thing that RocksDB seek behaves like a point query rather than a range query.

We can do something similar for the delete as well and add delete range tombstone as well.

The lifecycle configuration service cannot use deleteRange (unlike DirectoryDeletingService since AFAIK DirectoryDeletingService only handles orphan directories and no keys will be created under the directory anymore) since another key can be created in the deleteRange which can cause valid keys to be inadvertently deleted, causing data loss.

Do you guys have ACL bloat problem?

We previously had one, but we already resolved it so that now a single key usually only have 1 ACL only. Our custom authorizer allows us to have a stable ACL per key.

But each OmKeyInfo can be large due to a lot of blocks (for example, if a key is uploaded using MPU with very small parts), so the risk is still there unless we make OmKeyInfo to not contain any unbounded list (i.e. ACLs and Blocks).

[swamirishi]

Is there some bulk delete in the buckets?

@swamirishi Yes, we have the S3 lifecycle configuration service (HDDS-8342) that generates a few millions deletions in a very short time. In LEGACY bucket getFileStatus can trigger a RocksDB seek that is done under a bucket lock, if there are a lot of tombstones, then the lock will be held for a long time which can block Ratis applier and cause the OM write to be stuck. We have resolved it in #9932 by moving the createFakeDirIfShould outside the lock, but new patches can easily fall to the same trap since some people might thing that RocksDB seek behaves like a point query rather than a range query.

We can do something similar for the delete as well and add delete range tombstone as well.

The lifecycle configuration service cannot use deleteRange (unlike DirectoryDeletingService since AFAIK DirectoryDeletingService only handles orphan directories and no keys will be created under the directory anymore) since another key can be created in the deleteRange which can cause valid keys to be inadvertently deleted, causing data loss.

No that should not happen as long as you decide to do the iteration and delete range inside validateAndUpdateCache method of

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

Line 117 in daef2f1

public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) {

We can sort the keys to be deleted and initialize a rocksdb iterator for key table and do delete range for contiguous keys to be deleted in the iterator. Basically have a 2 pointer iteration b/w the keys to be deleted and the rocksdb iterator this would figure out the contiguous keys to be deleted. If the million of keys are contiguous it would optimize the number of tombstones we create by a huge magnitude. Basically I would propose to change this unoptimal loop which does individual seeks and get for each delete key which can be very unoptimal in every case having a rocksdb iterator would be way more optimal.

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

Lines 187 to 192 in daef2f1

	for (indexFailed = 0; indexFailed < length; indexFailed++) {
	String keyName = deleteKeyArgs.getKeys(indexFailed);
	String objectKey =
	omMetadataManager.getOzoneKey(volumeName, bucketName, keyName);
	OmKeyInfo omKeyInfo = getOmKeyInfo(ozoneManager, omMetadataManager,
	volumeName, bucketName, keyName);

I am assuming you guys are using the bulk delete api to delete since I don't think it would be possible to do millions of deletes when deleting individual keys anyhow because of our om ratis constraints.

Do you guys have ACL bloat problem?

We previously had one, but we already resolved it so that now a single key usually only have 1 ACL only. Our custom authorizer allows us to have a stable ACL per key.

But each OmKeyInfo can be large due to a lot of blocks (for example, if a key is uploaded using MPU with very small parts), so the risk is still there unless we make OmKeyInfo to not contain any unbounded list (i.e. ACLs and Blocks).

[swamirishi]

Is there some bulk delete in the buckets?

@swamirishi Yes, we have the S3 lifecycle configuration service (HDDS-8342) that generates a few millions deletions in a very short time. In LEGACY bucket getFileStatus can trigger a RocksDB seek that is done under a bucket lock, if there are a lot of tombstones, then the lock will be held for a long time which can block Ratis applier and cause the OM write to be stuck. We have resolved it in #9932 by moving the createFakeDirIfShould outside the lock, but new patches can easily fall to the same trap since some people might thing that RocksDB seek behaves like a point query rather than a range query.

We can do something similar for the delete as well and add delete range tombstone as well.

The lifecycle configuration service cannot use deleteRange (unlike DirectoryDeletingService since AFAIK DirectoryDeletingService only handles orphan directories and no keys will be created under the directory anymore) since another key can be created in the deleteRange which can cause valid keys to be inadvertently deleted, causing data loss.

No that should not happen as long as you decide to do the iteration and delete range inside validateAndUpdateCache method of

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

Line 117 in daef2f1

public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) {

We can sort the keys to be deleted and initialize a rocksdb iterator for key table and do delete range for contiguous keys to be deleted in the iterator. Basically have a 2 pointer iteration b/w the keys to be deleted and the rocksdb iterator this would figure out the contiguous keys to be deleted. If the million of keys are contiguous it would optimize the number of tombstones we create by a huge magnitude. Basically I would propose to change this unoptimal loop which does individual seeks and get for each delete key which can be very unoptimal in every case having a rocksdb iterator would be way more optimal.

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

Lines 187 to 192 in daef2f1

	for (indexFailed = 0; indexFailed < length; indexFailed++) {
	String keyName = deleteKeyArgs.getKeys(indexFailed);
	String objectKey =
	omMetadataManager.getOzoneKey(volumeName, bucketName, keyName);
	OmKeyInfo omKeyInfo = getOmKeyInfo(ozoneManager, omMetadataManager,
	volumeName, bucketName, keyName);

I am assuming you guys are using the bulk delete api to delete since I don't think it would be possible to do millions of deletes when deleting individual keys anyhow because of our om ratis constraints.

Do you guys have ACL bloat problem?

We previously had one, but we already resolved it so that now a single key usually only have 1 ACL only. Our custom authorizer allows us to have a stable ACL per key.
But each OmKeyInfo can be large due to a lot of blocks (for example, if a key is uploaded using MPU with very small parts), so the risk is still there unless we make OmKeyInfo to not contain any unbounded list (i.e. ACLs and Blocks).

I see you already have a ticket for this https://issues.apache.org/jira/browse/HDDS-14613 . Let us work on this particular change.

[ivandika3]

No that should not happen as long as you decide to do the iteration and delete range inside validateAndUpdateCache method of

@swamirishi This will kill OM performance, we cannot use RocksDB iterator in validateAndUpdateCache (which can run for a long time due to tombstones) and split them into multiple deleteRange groups since it will block subsequent writes. The lifecycle configuration scanning should not be run in a critical path.

We can sort the keys to be deleted and initialize a rocksdb iterator for key table and do delete range for contiguous keys to be deleted in the iterator. Basically have a 2 pointer iteration b/w the keys to be deleted and the rocksdb iterator this would figure out the contiguous keys to be deleted. If the million of keys are contiguous it would optimize the number of tombstones we create by a huge magnitude. Basically I would propose to change this unoptimal loop which does individual seeks and get for each delete key which can be very unoptimal in every case having a rocksdb iterator would be way more optimal.

We cannot assume that the majority of keys are contiguous. If we have millions of noncontiguous keys, the deleteRange will just added overhead.

Basically I would propose to change this unoptimal loop which does individual seeks and get for each delete key which can be very unoptimal in every case having a rocksdb iterator would be way more optimal

Seek and Get is different, seek implies a range query (i.e. RocksDB iterator which can run for a very long time). Get is a point query that runs a lot faster (due to mechanisms like bloom filter). The current OM deletion only runs point query.

I see you already have a ticket for this https://issues.apache.org/jira/browse/HDDS-14613 . Let us work on this particular change.

I closed that for the reason above.

[swamirishi]

No that should not happen as long as you decide to do the iteration and delete range inside validateAndUpdateCache method of

@swamirishi This will kill performance, we cannot iterate every single key in validateAndUpdateCache and split them into multiple deleteRange groups since it will block subsequent writes. The lifecycle configuration scanning should not be run in a critical path.

I see you already have a ticket for this https://issues.apache.org/jira/browse/HDDS-14613 . Let us work on this particular change.

I closed that for the reason above.

Not exactly IMO the iteration that we are doing right now rather is very unoptimal. The actual algo I am proposing would be way more optimal and would run something like this:
Say we want to delete
k01, k02, k04, k05, k10, k11
and the key table has keys from k00..k99

1. Init keyTable rocksdb iterator. Create a tmpList to keep track of OmKeyInfos and deleteRangeList
2. The first key in iterator is k00 and delete keys is k01. If pointer at deleted key is greater than iterator then iterator seeks to deleted key by adding the current deleteRange to the deleteRangeList. So the iterator is at k01 now.
3. Both iterator is k01 and delete key is k01 so add k01 to OmKeyInfoList and move pointer of delete Key list to k02 and itr.next(). Similarly K02 will match and would be added as well and we proceed to next.
4. Now DeleteKey is at K04 and rocksdbIterator is at k03. So we add deleteKeyRange [k01, k03) in deleteKeyRangeList and seek to k04.
   The algo proceeds on like this till we don't iterate through all keys in the deleteKeyList. This would be much more optimal than whatever we are doing right now since every rocksdb get is going to be an equivalent of randomized get in the LSM which would be very unoptimal incomparison to sequential gets which happens in an iterator.

[ivandika3]

The first key in iterator is k00 and delete keys is k01. If pointer at deleted key is greater than iterator then iterator seeks to deleted key by adding the current deleteRange to the deleteRangeList. So the iterator is at k01 now.

Let's say that between k00 and k01 as well as k02 and k04, there are 1 million tombstones, the RocksDB iterator is forced to iterate all these 1 million tombstone entries. This will make performance unpredictable.

The deleteRange on [k01, k03) is not going to be worth it in that case.

There is a RocksDB multiget https://github.com/facebook/rocksdb/wiki/MultiGet-Performance to improve performance of multiget, we can probably explore this, but I don't think the proposed algorithm above is the way to go.

The rule of thumb for me is to never use range query (even if it's implicit like RocksDB seek) if you are planning to do point query / queries.

@swamirishi Anyway, we've digressed from the purpose of the original implementation, let's continue the discussion in HDDS-14613

[swamirishi]

The first key in iterator is k00 and delete keys is k01. If pointer at deleted key is greater than iterator then iterator seeks to deleted key by adding the current deleteRange to the deleteRangeList. So the iterator is at k01 now.

Let's say that between k00 and k01 as well as k02 and k04, there are 1 million tombstones, the RocksDB iterator is forced to iterate all these 1 million tombstone entries. This will make performance unpredictable.

The deleteRange on [k01, k03) is not going to be worth it in that case.

There is a RocksDB multiget https://github.com/facebook/rocksdb/wiki/MultiGet-Performance to improve performance of multiget, we can probably explore this, but I don't think the proposed algorithm above is the way to go.

The rule of thumb for me is to never use range query (even if it's implicit like RocksDB seek) if you are planning to do point query / queries.

Seek should be a relatively safe. But I just realized we cannot use deleteRange because of snapshots since snapshot diff depends on the individual tombstone entry for deletes.

@swamirishi Anyway, we've digressed from the purpose of the original implementation, let's continue the discussion in HDDS-14613

Yeah sure. As regards to this change I am fine with the change. I believe we should change the Compaction type to kForceOptimized.