Skip to content

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Jul 2, 2025

This change adds a new abstract class ElasticsearchIndexDeletionPolicy that allows using different policy implementations for InternalEngine.

It also adds an index deletion policy wrapper to the EngineConfig to allow wrapping the default CombinedDeletionPolicy by inheritors of InternalEngine.

Relates ES-10929

@tlrx tlrx added >non-issue :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Jul 2, 2025
@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Jul 2, 2025
Copy link
Contributor

@kingherc kingherc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work. Some early comments.


public abstract boolean hasUnreferencedCommits();

public interface CommitsListener {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about getting rid of this interface since we don't use it anymore, wdyt @kingherc? we can do in a follow-up once all the pieces are in place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fcofdez . Yes I believe it can be removed.

Separately, we were discussing whether IndexModule#setIndexCommitListener() could be removed in this thread with @tlrx , feel free to read to see as well. I had a question whether someone, e.g., externally, like a plugin writer user, might have been using it, but we think no. So that may also be considered for removal. However, maybe an optional follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that was my impression too. I'll remove it in a follow-up so we keep this PRs manageable.

@fcofdez fcofdez requested a review from kingherc July 16, 2025 13:08
@fcofdez fcofdez marked this pull request as ready for review July 16, 2025 13:08
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Indexing Meta label for Distributed Indexing team label Jul 16, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

Copy link
Contributor

@kingherc kingherc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


public abstract boolean hasUnreferencedCommits();

public interface CommitsListener {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fcofdez . Yes I believe it can be removed.

Separately, we were discussing whether IndexModule#setIndexCommitListener() could be removed in this thread with @tlrx , feel free to read to see as well. I had a question whether someone, e.g., externally, like a plugin writer user, might have been using it, but we think no. So that may also be considered for removal. However, maybe an optional follow-up.

@fcofdez fcofdez merged commit c0facac into elastic:main Jul 28, 2025
33 checks passed
szybia added a commit to szybia/elasticsearch that referenced this pull request Jul 28, 2025
…-tracking

* upstream/main:
  Fix MergeWithLowDiskSpaceIT testRelocationWhileForceMerging (elastic#131806)
  [ML] Prevent the trained model deployment memory estimation from double-counting allocations. (elastic#131990)
  ES|QL Assert current thread during query planning and execution (elastic#131807)
  Add ElasticsearchIndexDeletionPolicy and EngineConfig policy wrapper (elastic#130442)
  [TEST] Adds tests for ESTestCase randomSubset methods (elastic#131745)
  Simplify esql session (elastic#131925)
  Simplify EsqlExecution info serialization (elastic#131823)
  Add utility to check for project global block (elastic#131927)
  [DOCS] Update ES|QL applies to's (elastic#131805)
  Handle structured log messages (elastic#131027)
  Mute org.elasticsearch.test.rest.yaml.RcsCcsCommonYamlTestSuiteIT test {p0=search/600_flattened_ignore_above/flattened ignore_above multi-value field} elastic#131967
  Mute org.elasticsearch.xpack.remotecluster.CrossClusterEsqlRCS2EnrichUnavailableRemotesIT testEsqlEnrichWithSkipUnavailable elastic#131965
  Mute org.elasticsearch.xpack.restart.FullClusterRestartIT testWatcherWithApiKey {cluster=UPGRADED} elastic#131964
  [ES|QL] Fix aggregate_metric_double sorting and mv_expand issues (elastic#131658)
  Reduce logging levels for meter usage tests (elastic#131935)
afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Jul 28, 2025
…lastic#130442)

This change adds a new abstract class ElasticsearchIndexDeletionPolicy that allows using different policy implementations for InternalEngine.

It also adds an index deletion policy wrapper to the EngineConfig to allow wrapping the default CombinedDeletionPolicy by inheritors of InternalEngine.

Relates ES-10929

---------

Co-authored-by: Francisco Fernández Castaño <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >non-issue serverless-linked Added by automation, don't add manually Team:Distributed Indexing Meta label for Distributed Indexing team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants