-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[CPS] Search with MRT=true #137701
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
[CPS] Search with MRT=true #137701
Conversation
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
quux00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial feedback after first round review.
server/src/main/java/org/elasticsearch/action/ResolvedIndices.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/ResolvedIndices.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Show resolved
Hide resolved
quux00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress. Overall very clean code. This was a first pass review. I'll want to dive deep on the next round also once the first round of feedback is discussed.
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/ResolvedIndices.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
quux00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on removing the local resolution via _resolve/index. That part looks solid.
I left a few questions/suggestions that are still confusing me.
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Show resolved
Hide resolved
|
|
||
| originalResolvedIndices.getRemoteClusterIndices() | ||
| .forEach( | ||
| (projectName, projectIndices) -> resolveRemoteCrossProjectIndex( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a follow-on PR, I think we'll need to build some tests that simulate the case where one of the remote clusters is not available (not responding) and hits the timeout. We would do those tests in the serverless IT test and we can use some of the test plugin features we have in the core CCS IT tests to induce non-responsive linked projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a plugin for it. It is achievable by adding a "handling behaviour" to the underlying MockTransportService.
| /** | ||
| * Only used for ccs_minimize_roundtrips=true pathway | ||
| */ | ||
| private void mergeResolvedIndices( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(optional) - I think you could make this method static (and non-private) so that you could write unit tests for it.
That may not be worth it since it does it 3 things and two of those (CrossProjectIndexResolutionValidator.validate and ResolvedIndices.resolveWithIndexExpressions) are already static methods with tests.
| new OriginalIndices(new String[] { "some-local-index" }, IndicesOptions.DEFAULT), | ||
| Map.of(mock(), mock()), | ||
| remoteExpressions, | ||
| IndicesOptions.DEFAULT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the IndicesOptions we'll be passing in for CPS? If not, should we change it to match what the production code passes in (which I think would be from indicesOptionsForCrossProjectFanout)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the production code, it's resolutionIdxOpts, so that's the CPS options. So, yes, it'd be wiser to use them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it passes as-is - it can be any value
if it needs to be a specific value, should we add validation or an assertion (to both MRT = false and MRT = true paths)?
|
|
||
| /** | ||
| * Create a new {@link ResolvedIndices} instance from a Map of Projects to {@link ResolvedIndexExpressions}. | ||
| * This method guarantees that the resulting remote project contains at least one index resolved for the mapped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like I'm missing something. This statement:
This method guarantees that the resulting remote project contains at least one index resolved for the mapped ResolvedIndexExpressions
doesn't seem right to me. The check that guarantees this is the call to CrossProjectIndexResolutionValidator.validate in mergeResolvedIndices, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a guarantee on the map's (remoteIndices) structuring rather than a behaviour ascertaining. Either way, my opinion is that it's not immediately clear what the doc means, unless you're by default familiar with the CPS specifics. Could this be rewritten or clarified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 268 through 280 iterates over a map of ResolvedIndexExpressions and filters the internal list of ResolvedIndexExpression for entities that have .localIndexResolutionResult() == SUCCESS and at least one index.
For example, if project-1 had the original index expression logs* map to indices ["logs-1", "logs-2"], then the result has project-1 map to ["logs*"]. If project-1 had logs* map to [], then project-1 is not included in the resulting map.
So, each remote project that is keyed in the resulting ResolvedIndices has at least one index that is mapped from the original index expression.
The thing I would like to verify is the assumption that we made in the slack thread, where we set the remote project's index in the OriginalIndices to be the original index expression and not the resolved index (e.g. ["logs*"] and not ["logs-1", "logs-2"].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing I would like to verify is the assumption that we made in the slack thread, where we set the remote project's index in the OriginalIndices to be the original index expression and not the resolved index
That's right. We want to associate with the user input index expressions and not the resolved values. You're on the right track.
| import static org.hamcrest.Matchers.not; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
||
| public class ResolvedIndicesTests extends ESTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ResolvedIndices.resolveWithIndexExpressions the method that ensures the right subset of indices are assigned to each linked project?
In other words, if I have say GET logs,foo,bar/_search and two linked projects p1 and p2, suppose these are the only indices that exist across all 3 projects:
_origin: logs
p1:foo
p2:bar
is ResolvedIndices.resolveWithIndexExpressions the method that ensures that each project alias is mapped to only the indices/aliases/datastreams found on each project?
If yes, I'd like to see that scenario tested here. I think you are only testing on one linked project and I'm having trouble following both what ResolvedIndices.resolveWithIndexExpressions does (the code is very dense) and what is being tested here. I think laying it out with code comments would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is ResolvedIndices.resolveWithIndexExpressions the method that ensures that each project alias is mapped to only the indices/aliases/datastreams found on each project?
kinda, from our discussion in the thread, it maps to the original index expression that resolves to the indices/aliases/datastreams found on each project.
"once we validated the flat expression we can use the rewritten index expression, so p1:logs* is fine no need to change it to p1:logs-1"
pawankartik-elastic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with how the changes turned out. I'll approve it shortly.
server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
Show resolved
Hide resolved
pawankartik-elastic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I don't see anything blocking my approval.
quux00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - great work here Pat!
Search when MinimizeRoundTrip (MRT) and Cross-Project Search (CPS) are true. This will fan out to call ResolveIndexAction on each resolved LinkedProject. Results are merged and validated based on
ignore_unavailableandallow_no_indices.The results are used as the new
resolvedIndiceswhen issuing the subsequent search requests. Linked projects will be searched using the original index express.