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.

[ptlrs]

Thanks for the PR @ivandika3. Removing this configuration should be ok. As per the RocksDB source, exclusive_manual_compaction=true is not required since 2021/prior to 7.7.3.

  // When a manual compaction arrives, temporarily disable scheduling of
  // non-manual compactions and wait until the number of scheduled compaction
  // jobs drops to zero. This used to be needed to ensure that this manual
  // compaction can compact any range of keys/files. Now it is optional
  // (see `CompactRangeOptions::exclusive_manual_compaction`). The use case for
  // `exclusive_manual_compaction=true` is unclear beyond not trusting the code.

• https://github.com/facebook/rocksdb/blob/01cd296b079d1c82e28bfddd6850d7de876aaf6d/db/db_impl/db_impl_compaction_flush.cc#L1858-L1864
• Allow manual compactions to run in parallel by default facebook/rocksdb#10317

This scenario has essentially stalled writes by creating a backpressure in the write flow. While removing this configuration helps, the scenario may still reproduce if L0 to L1 compaction slows down due to any other reason. We may need to revisit the values for max_subcompactions, max_background_compactions and max_background_jobs.

[ivandika3]

Thanks @ptlrs

As per the RocksDB source, exclusive_manual_compaction=true is not required since 2021/prior to 7.7.3.

Yes this means that there is no point of adding exclusive_manual_compaction and we just added this option arbitrarily.

This scenario has essentially stalled writes by creating a backpressure in the write flow. While removing this configuration helps, the scenario may still reproduce if L0 to L1 compaction slows down due to any other reason. We may need to revisit the values for max_subcompactions, max_background_compactions and max_background_jobs.

Yes please do revisit this. As mentioned before, yes even the L0 to L1 compaction can slow down if the machine resources is already fully saturated which slow down the compaction so that the write speed overwhelm the compaction speed, in which case the backpressure mechanism is valid to avoid disk out of space issue (which is a more catastrophic issue). However, the exclusive_manual_compaction forcefully slow down the compaction even if the machine has resources to run other compactions.

[ivandika3]

believe we should change the Compaction type to kForceOptimized.

@swamirishi please provide the full reasoning why kForceOptimized is better.

Seek should be a relatively safe

Also please provide reasoning how this is "relatively safe", relative to what? Unless seek is a point query and will not need to iterate over tombstones, seek is unsafe.

But I just realized we cannot use deleteRange because of snapshots since snapshot diff depends on the individual tombstone entry for deletes.

Then all the deleteRange patch to OM need to be prohibited until snapshot feature handles it.

Regarding the multi-get idea, I don't think it's possible since OM still uses the table cache and RocksDB cannot update both the table cache (which is Ozone logic) and the RocksDB itself. In that case, we can consider whether this OM table cache (double buffer, etc) is actually worth the effort or just another premature optimization (we can check how other Raft + RocksDB system like TiKV actually does it). We can then consider removing OM table cache entirely. Table cache is also a source of inconsistency in OM since linearizability seem to be violated (see #10748) and I don't think anyone (including me) actually knows the consistency guarantee to the table cache since the design document never specifies it.

[swamirishi]

Then all the deleteRange patch to OM need to be prohibited until snapshot feature handles it.

believe we should change the Compaction type to kForceOptimized.

@swamirishi please provide the full reasoning why kForceOptimized is better.

Seek should be a relatively safe

Also please provide reasoning how this is "relatively safe", relative to what? Unless seek is a point query and will not need to iterate over tombstones, seek is unsafe.

When you do seek it doesn't iterate through all the tombstones it would try to search in the LSM using indexes in the memtable and ssts to efficiently figure out the keys. Only iterator.next() would have troubles when there are lot of tombstones. With deleteRange we are actually solving the tombstone problem and thus solving the slow iterator problem. With deleteRange the iterator would skip an entire range instead of having to go through each tombstone on by one.

But I just realized we cannot use deleteRange because of snapshots since snapshot diff depends on the individual tombstone entry for deletes.

Then all the deleteRange patch to OM need to be prohibited until snapshot feature handles it.

We cannot support DeleteRange on the user path on keyTable/DirTable/FileTable(background services are ok) till the point non defragmented snapshot diff doesn't understand deleteRange tombstones. If we add a simple loop to identify the keys with deleteRange on the snapshot diff flows we can find the diff then.

Regarding the multi-get idea, I don't think it's possible since OM still uses the table cache and RocksDB cannot update both the table cache (which is Ozone logic) and the RocksDB itself. In that case, we can consider whether this OM table cache (double buffer, etc) is actually worth the effort or just another premature optimization (we can check how other Raft + RocksDB system like TiKV actually does it). We can then consider removing OM table cache entirely. Table cache is also a source of inconsistency in OM since linearizability seem to be violated (see #10748) and I don't think anyone (including me) actually knows the consistency guarantee to the table cache since the design document never specifies it.

[ivandika3]

When you do seek it doesn't iterate through all the tombstones it would try to search in the LSM using indexes in the memtable and ssts to efficiently figure out the keys. Only iterator.next() would have troubles when there are lot of tombstones. With deleteRange we are actually solving the tombstone problem and thus solving the slow iterator problem. With deleteRange the iterator would skip an entire range instead of having to go through each tombstone on by one.

As I have iterated before, seek() is a range query and will iterate over tombstones, not a point query like get() where it will "search in the LSM using indexes in the memtable and ssts to efficiently figure out the keys". I understand that if we use deleteRange, there will be less of these tombstones since they will be coalesced to a single deleteRange tombstones, but the seek and iteration during validateAndUpdateCache still introduces unacceptable performance impact.

believe we should change the Compaction type to kForceOptimized.

This remains unaddressed.

[swamirishi]

RocksDB source, exclusive_manual_compaction=true is not required since 2021/prior to 7.7.3.

believe we should change the Compaction type to kForceOptimized.

@swamirishi please provide the full reasoning why kForceOptimized is better.

KforceOptimized potentially reduces writeAmplifications by ensuring the files which were newly generated in the compaction doesn't get redundantly get compacted again.

Seek should be a relatively safe

Also please provide reasoning how this is "relatively safe", relative to what? Unless seek is a point query and will not need to iterate over tombstones, seek is unsafe.

But I just realized we cannot use deleteRange because of snapshots since snapshot diff depends on the individual tombstone entry for deletes.

Then all the deleteRange patch to OM need to be prohibited until snapshot feature handles it.

Regarding the multi-get idea, I don't think it's possible since OM still uses the table cache and RocksDB cannot update both the table cache (which is Ozone logic) and the RocksDB itself. In that case, we can consider whether this OM table cache (double buffer, etc) is actually worth the effort or just another premature optimization (we can check how other Raft + RocksDB system like TiKV actually does it). We can then consider removing OM table cache entirely. Table cache is also a source of inconsistency in OM since linearizability seem to be violated (see #10748) and I don't think anyone (including me) actually knows the consistency guarantee to the table cache since the design document never specifies it.

[swamirishi]

When you do seek it doesn't iterate through all the tombstones it would try to search in the LSM using indexes in the memtable and ssts to efficiently figure out the keys. Only iterator.next() would have troubles when there are lot of tombstones. With deleteRange we are actually solving the tombstone problem and thus solving the slow iterator problem. With deleteRange the iterator would skip an entire range instead of having to go through each tombstone on by one.

As I have iterated before, seek() is a range query and will iterate over tombstones, not a point query like get() where it will "search in the LSM using indexes in the memtable and ssts to efficiently figure out the keys".

believe we should change the Compaction type to kForceOptimized.

This remains unaddressed.

When you do seek it doesn't iterate through all the tombstones it would try to search in the LSM using indexes in the memtable and ssts to efficiently figure out the keys. Only iterator.next() would have troubles when there are lot of tombstones. With deleteRange we are actually solving the tombstone problem and thus solving the slow iterator problem. With deleteRange the iterator would skip an entire range instead of having to go through each tombstone on by one.

As I have iterated before, seek() is a range query and will iterate over tombstones, not a point query like get() where it will "search in the LSM using indexes in the memtable and ssts to efficiently figure out the keys". I understand that if we use deleteRange, there will be less of these tombstones since they will be coalesced to a single deleteRange tombstones, but the seek and iteration during validateAndUpdateCache still introduces unacceptable performance impact.

I am not sure if you understood the point I was making. Let us go through an example of an iterator.
L0: A.sst [1500-3500] B.sst [3000-4000] also has deleteRange of [1000-1500)
L1: D.sst [1000-2000], F.sst [2001-5000]
L2: F.sst [1500-3000]
L6 : Z.sst[0000- 9000]
So when we initialize an iterator and rocksdb would internally create a merge iterator consisting of multiple sstFileIterator one for each on L0 and one sstFileIterator for each level. Say when you do seek to 1300. Then the sstFileIterator in the lower levels in the LSM tree would automatically jump to 1500 since there is a range tombstone in L0. So the logic of CPU burn is false, it would be a problem only when there are too many L0 files which is not the problem in our case. With the proposed changes we would not have a lot of tombstones to begin with and only multiple range tombstones which would not be in big numbers thus the iterator overhead would be minimal not as major as you would think.
So in the above case a seek to 1300 would have the following sstFileIterator states:
L0: A(1300), B(3000)
L1: D(1500)
L2: F(1500)
L6: Z(1500)
So D,F and Z iterators would have completely skipped [1300-1500) it wouldn't have to decode the blocks corresponding to these keys as these are not relevant it would directly do a seek to 1500.
Moreover with range tombstones we are also reducing the size of our memtable thus indirectly reducing the number of L0 sst files. It is beneficial in every sense performance wise.

believe we should change the Compaction type to kForceOptimized.

This remains unaddressed.

[ivandika3]

KforceOptimized potentially reduces writeAmplifications by ensuring the files which were newly generated in the compaction doesn't get redundantly get compacted again.

Ok, sounds valid.

Let us go through an example of an iterator.

Thanks for the explanation. I see the benefit of deleteRange if most a lot of large ranges are covered by deleteRange.

With the proposed changes we would not have a lot of tombstones to begin with and only multiple range tombstones which would not be in big numbers thus the iterator overhead would be minimal not as major as you would think

This cannot not be assumed, if user sends a lot of delete requests, tombstones might still be a lot. Also for existing cluster which high tombstones, the overhead still happens.

@swamirishi Btw, please help to approve this if you think it looks good so I can merge it.

[swamirishi]

LGTM @ivandika3 thanks for working on the patch