Skip to content

Commit 5a1d9f3

Browse files
authored
Try if tombstone is eligable for pruning before locking on it's key (#28767)
Pruning tombstones is quite expensive since we have to walk though all deletes in the live version map and acquire a lock on every value even though it's impossible to prune it. This change does a pre-check if a delete is old enough and if not it skips acquireing the lock.
1 parent f53d159 commit 5a1d9f3

File tree

1 file changed

+23
-17
lines changed

1 file changed

+23
-17
lines changed

server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -375,25 +375,31 @@ void removeTombstoneUnderLock(BytesRef uid) {
375375
}
376376
}
377377

378+
private boolean canRemoveTombstone(long currentTime, long pruneInterval, DeleteVersionValue versionValue) {
379+
// check if the value is old enough to be removed
380+
final boolean isTooOld = currentTime - versionValue.time > pruneInterval;
381+
// version value can't be removed it's
382+
// not yet flushed to lucene ie. it's part of this current maps object
383+
final boolean isNotTrackedByCurrentMaps = versionValue.time < maps.getMinDeleteTimestamp();
384+
return isTooOld && isNotTrackedByCurrentMaps;
385+
}
386+
378387
void pruneTombstones(long currentTime, long pruneInterval) {
379388
for (Map.Entry<BytesRef, DeleteVersionValue> entry : tombstones.entrySet()) {
380-
final BytesRef uid = entry.getKey();
381-
try (Releasable lock = keyedLock.tryAcquire(uid)) {
382-
// we use tryAcquire here since this is a best effort and we try to be least disruptive
383-
// this method is also called under lock in the engine under certain situations such that this can lead to deadlocks
384-
// if we do use a blocking acquire. see #28714
385-
if (lock != null) { // did we get the lock?
386-
// can we do it without this lock on each value? maybe batch to a set and get the lock once per set?
387-
// Must re-get it here, vs using entry.getValue(), in case the uid was indexed/deleted since we pulled the iterator:
388-
final DeleteVersionValue versionValue = tombstones.get(uid);
389-
if (versionValue != null) {
390-
// check if the value is old enough to be removed
391-
final boolean isTooOld = currentTime - versionValue.time > pruneInterval;
392-
if (isTooOld) {
393-
// version value can't be removed it's
394-
// not yet flushed to lucene ie. it's part of this current maps object
395-
final boolean isNotTrackedByCurrentMaps = versionValue.time < maps.getMinDeleteTimestamp();
396-
if (isNotTrackedByCurrentMaps) {
389+
// we do check before we actually lock the key - this way we don't need to acquire the lock for tombstones that are not
390+
// prune-able. If the tombstone changes concurrently we will re-read and step out below since if we can't collect it now w
391+
// we won't collect the tombstone below since it must be newer than this one.
392+
if (canRemoveTombstone(currentTime, pruneInterval, entry.getValue())) {
393+
final BytesRef uid = entry.getKey();
394+
try (Releasable lock = keyedLock.tryAcquire(uid)) {
395+
// we use tryAcquire here since this is a best effort and we try to be least disruptive
396+
// this method is also called under lock in the engine under certain situations such that this can lead to deadlocks
397+
// if we do use a blocking acquire. see #28714
398+
if (lock != null) { // did we get the lock?
399+
// Must re-get it here, vs using entry.getValue(), in case the uid was indexed/deleted since we pulled the iterator:
400+
final DeleteVersionValue versionValue = tombstones.get(uid);
401+
if (versionValue != null) {
402+
if (canRemoveTombstone(currentTime, pruneInterval, versionValue)) {
397403
removeTombstoneUnderLock(uid);
398404
}
399405
}

0 commit comments

Comments
 (0)