Should we put this setting in IndexSettings.java instead? Also, if this setting is dynamic, we should invalidate the cached value when it is updated.

The Property.Dynamic property was leftover from an earlier iteration, removed in f806892, thanks. Yes you're right if we were consuming updates then we should also be invalidating the cache.

I'm not sure where we should declare this setting. It's basically a constant (except for tests) and this is where I'd naturally declare a constant.

Moved in to IndexSettings in c220530 since that's about the most sensible place for it in the light of that change.

Perhaps use SegmentInfos#getId instead of the generation of the commit as the cache key. We can have the same generation for different index commits if we perform a file-based recovery.

Yes, 763a944.

Releasing a commit is quite expensive. Maybe expose a new method that returns the safe commit without acquiring. I think it's fine to use the last commit (we already exposed it) instead of the safe commit here.

Please ignore this as it is not correct. We won't revisit the deletion policy on releasing a commit unless the safe commit advances.

I too am a little uneasy with the costs of this. With this implementation each call to getAsLong() is synchronized and obtains a new commit ref, which might occasionally be expensive to release. It only does this on replication groups that aren't fully-assigned, of course. An alternative would be to recalculate this while advancing the safe commit, then this could just become a volatile read. I couldn't determine a neat way to hook this calculation in the right place. Any ideas?

I tried moving the calculation to CombinedDeletionPolicy#updateRetentionPolicy in c220530. I think it makes sense there. WDYT?

Maybe use Lucene.readSegmentInfos(safeCommit).totalMaxDoc()?

Ah thanks, 9cd3c80.

Drive by comment: I think this will do IO while holding the monitor. This would then prevent concurrent acquireIndexCommit and releaseIndexCommit calls. A quick scrape of usages did not find any where this would hurt, but I would anyway suggest to do the calculation of minimumReasonableRetainedSeqNo outside the monitor instead.

Yes, it will. We're already in a method that throws IOException but you're right, it doesn't look like it really does do much IO as it currently stands. Moved outside the mutex in 5860c41.

I wonder if this should be called index.recovery.full_recovery_threshold or index.recovery.file_based_threshold.
It's essentially a threshold beyond which full file-based recoveries are preferred.
Same goes for the variable names throughout the rest of the PR

Ok, changed to index.recovery.file_based_threshold in 13a4c38.

what was the reason to compute this value here and not in ReplicationTracker? This class could e.g. return a pre-computed safeCommitInfo that contains local checkpoint + number of docs.
The actual computation could then be done in replication tracker, where I think that logic fits more naturally (and avoids the slightly odd minimumReasonableRetainedSeqNo naming that we have here).

It didn't really seem worth the extra plumbing to move this single expression, particularly since CombinedDeletionPolicy also has some bearing on history retention. It didn't seem any more natural to have it in ReplicationTracker. But I've done this in 13a4c38.

Do we really need this separate mutex?

I am not sure, because it depends on whether onCommit is called concurrently or not. Is it? If it is then we could end up with stale data in safeCommitInfo.

Before this change, onCommit was a synchronized method. Is it okay if we leave it as is?

Henning asked to avoid running the IO needed to compute the safeCommitInfo under this mutex here: #45208 (comment).

Ah, ok. Although It's not an issue, I think we should avoid having two lock orderings:
this -> onCommitMutex in onInit and onCommitMutex -> this in onCommit. How about implementing onCommit in two steps using a single mutex.

final IndexCommit safeCommit;
synchronized (this) {
	// update the policy
        safeCommit = this.safeCommit;
}
final SafeCommitInfo safeCommitInfo = ..
synchronized (this) {
	if (safeCommit == this.safeCommit) {
		this.safeCommitInfo = safeCommitInfo;
	}
}

Ah, ok. Although It's not an issue, I think we should avoid having two lock orderings:
this -> onCommitMutex in onInit and onCommitMutex -> this in onCommit. How about implementing onCommit in two steps using a single mutex.

final IndexCommit safeCommit;
synchronized (this) {
	// update the policy
        safeCommit = this.safeCommit;
}
final SafeCommitInfo safeCommitInfo = ..
synchronized (this) {
	if (safeCommit == this.safeCommit) {
		this.safeCommitInfo = safeCommitInfo;
	}
}