@@ -375,25 +375,31 @@ void removeTombstoneUnderLock(BytesRef uid) {
375
375
}
376
376
}
377
377
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
+
378
387
void pruneTombstones (long currentTime , long pruneInterval ) {
379
388
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 )) {
397
403
removeTombstoneUnderLock (uid );
398
404
}
399
405
}
0 commit comments