Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify and flatten CanMatchPreFilterSearchPhase #118558

Conversation

original-brownbear
Copy link
Member

We don't need the result to be a SearchPhaseResults. In fact, there is no reason for it to be a nested class in the first place. Just flatten it into the phase itself and synchronize on this.

This is just a step on the way to #116881 that makes that PR much easier to review I believe.

We don't need the result to be a `SearchPhaseResults`. In fact,
there is no reason for it to be a nested class in the first place.
Just flatten it into the phase itself and synchronize on `this`.

This is just a step on the way to elastic#116881 that makes that PR much easier to review I believe.
@original-brownbear original-brownbear added >non-issue :Search Foundations/Search Catch all for Search Foundations labels Dec 12, 2024
@@ -521,7 +521,6 @@ void onShardFailure(final int shardIndex, SearchShardTarget shardTarget, Excepti
successfulOps.decrementAndGet(); // if this shard was successful before (initial phase) we have to adjust the counter
}
}
results.consumeShardFailure(shardIndex);
Copy link
Member Author

Choose a reason for hiding this comment

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

CanMatch had the only non-noop implementation of this method so it can go away now :)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v9.0.0 labels Dec 12, 2024
GroupShardsIterator<SearchShardIterator> shardsIts
) {
FixedBitSet possibleMatches = results.getPossibleMatches();
private synchronized GroupShardsIterator<SearchShardIterator> getIterator(GroupShardsIterator<SearchShardIterator> shardsIts) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This probably doesn't even need to be synchronized because we only call this after we passed a sync block for setting the last rresult, but given that we have one more step to go here that removes all sync anyway and with this not being contended to begin with, no need to take any risks here :)

listener.onFailure(new SearchPhaseExecutionException("can_match", msg, cause, ShardSearchFailure.EMPTY_ARRAY));
}

public Transport.Connection getConnection(SendingTarget sendingTarget) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was single use, no need to be tricky just inline it. This was a remnant of when can-match was an extension of the abstract async phase.

return nodeIdToConnection.apply(sendingTarget.clusterAlias, sendingTarget.nodeId);
}

private int getNumShards() {
Copy link
Member Author

Choose a reason for hiding this comment

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

No need for this, just inline it to the 3 uses, it's rather counterproductive to hide how we determine shard count to begin with IMO.

possibleMatches.set(shardIndex);
numPossibleMatches++;
}
minAndMaxes[shardIndex] = minAndMax;
Copy link
Member

Choose a reason for hiding this comment

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

What if we canMatch and have minAndMax==null? Can we override a previous minAndMaxes? I don't think so since is per shard but wanted to make sure.
And do we need to store minAndMax if canMatch is false? Not sure that's useful

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct on the minmax != null only for true but the code both before and after this PR does some redundant work here. This is all fixed in #116881, this PR is just meant to shorten that other PR and make those logical fixes easy to follow (in fact the min-max situation you point out is leveraged in that PR :) as an optimization).

Copy link
Member

@piergm piergm left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@original-brownbear original-brownbear added auto-backport Automatically create backport pull requests when merged v8.18.0 labels Jan 9, 2025
@original-brownbear
Copy link
Member Author

Thanks Matteo!

@original-brownbear original-brownbear merged commit b64b9ee into elastic:main Jan 9, 2025
16 checks passed
@original-brownbear original-brownbear deleted the simpler-can-match-prefilter branch January 9, 2025 22:18
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 118558

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged backport pending >non-issue :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants