Skip to content

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Sep 3, 2025

With doc_partitioning targeting large shards, updating the status of LuceneOperator can be expensive with many slices. This change uses the keys of the partitioning map instead of iterating over all slices in the queue.

@dnhatn
Copy link
Member Author

dnhatn commented Sep 3, 2025

update-status

@dnhatn dnhatn marked this pull request as ready for review September 3, 2025 19:34
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 3, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@dnhatn dnhatn requested a review from nik9000 September 3, 2025 19:36
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.

LGTM 👍

sb.append(this.getClass().getSimpleName()).append("[");
sb.append("shards = ").append(sortedUnion(processedShards, sliceQueue.remainingShardsIdentifiers()));
sb.append("shards = ")
.append(sliceQueue.partitioningStrategies().keySet().stream().sorted().collect(Collectors.joining(",", "[", "]")));
Copy link
Member

Choose a reason for hiding this comment

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

I think it was a mistake to stuff status-like-things things into toString. We have this information in the status already as a Map. Maybe we should just do getClass().getSimpleName() and call it good. If map_page_size isn't in the status we can put it there. This was all useful in a time before we had .status(). But we have it now and we don't need all the string stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

++ I can look into this later.

@dnhatn
Copy link
Member Author

dnhatn commented Sep 3, 2025

@martijnvg @nik9000 Thanks!

@dnhatn dnhatn merged commit c007e8c into elastic:main Sep 3, 2025
33 checks passed
@dnhatn dnhatn deleted the lucene-status branch September 3, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants