Skip to content

Conversation

@gmarouli
Copy link
Contributor

@gmarouli gmarouli commented Jul 4, 2025

Follow up from #130160 .

Use isSearchable instead of negating isPromotableToPrimary because it better captures what we need.

For non stateless deployments, we preserve the current logic to downsample the primary. This can change in the future but we should think through if there are any implications.

@gmarouli gmarouli requested a review from kingherc July 4, 2025 06:22
@gmarouli gmarouli added >refactoring :StorageEngine/Downsampling Downsampling (replacement for rollups) - Turn fine-grained time-based data into coarser-grained data labels Jul 4, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

One question, otherwise LGTM.

*/
private boolean isEligible(ShardRouting shardRouting) {
return shardRouting.started() && (isStateless ? shardRouting.isPromotableToPrimary() == false : shardRouting.primary());
return shardRouting.started() && (isStateless ? shardRouting.isSearchable() : shardRouting.primary());
Copy link
Member

Choose a reason for hiding this comment

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

In stateless, this would return false in indexing tier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the code I checked, this is determined based on the role of the shard, for example, an indexing shard is not searchable while a search shard is.

I think this should be sufficient, given that we trust that the searchable shard is assigned to the searching tier. In the end, downsampling needs a shard that can be searched, where that shard is allocated is a consequence of this, right?

Copy link
Member

Choose a reason for hiding this comment

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

Right and I think in case of stateless this is true.

@gmarouli gmarouli merged commit afb44b7 into elastic:main Jul 4, 2025
32 checks passed
@gmarouli gmarouli deleted the downsampling-runs-only-search-nodes-follow-up branch July 4, 2025 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>refactoring :StorageEngine/Downsampling Downsampling (replacement for rollups) - Turn fine-grained time-based data into coarser-grained data Team:StorageEngine v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants