-
Notifications
You must be signed in to change notification settings - Fork 25.7k
allows PIT to be cross project #137966
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
allows PIT to be cross project #137966
Conversation
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Hi @piergm, I've created a changelog YAML for you. |
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.
Initial comments.
server/src/main/java/org/elasticsearch/action/search/OpenPointInTimeRequest.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.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.
First pass review complete with suggestions and questions.
server/src/main/java/org/elasticsearch/action/search/RestOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
Outdated
Show resolved
Hide resolved
|
For the description comment:
Do we intend to tackle that in this PR or will be a follow-on? (Asking partly because we may not want Wei to do QA tests on this until close PIT is done?) |
luigidellaquila
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.
Thanks @piergm, I had a first look and I tested it with EQL, see my comment below
Really depends on when the non-Replaceable endpoints will allow UIAM. If it's early next week we can wait, otherwise I'd merge this and do a follow-up for PIT close. With that said when we merge this PR we can add to the E2E tests PIT open and search with PIT IMO. |
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.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.
LGTM.
I left a few questions and nits only.
| this.client = client; | ||
| this.crossProjectModeDecider = new CrossProjectModeDecider(clusterService.getSettings()); | ||
| this.forceConnectTimeoutSecs = clusterService.getSettings() | ||
| .getAsTime("search.ccs.force_connect_timeout", TimeValue.timeValueSeconds(3L)); |
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.
(asking to learn) - When is this set vs. taking the default of 3 seconds? I think for CPS we want to always ensure it is 3 seconds, so is search.ccs.force_connect_timeout always defined (and set to 3 seconds) for CPS or should we be using the CrossProjectModeDecider here to determine what timeout setting to use?
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's only being used by CPS because we only call it in executeOpenPitCrossProject, so I don't think we need to have another if statement for CrossProjectModeDecider.
On when/if is set I think we (ES eng) are the only one that can override/set this setting through gitops repo
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.
When is this set
When we manually "inject" the corresponding value from our end for Serverless CPS. I had a brief chat with Matteo about this, and there's scope for improvement/refactoring here: we'll abstract this away and move it elsewhere so that we won't have to:
- Hardcode the setting in
nnumber of places across the codebase, - Repeatedly check for the existence of this setting, and,
- Repeatedly declare the fallback values.
Currently, it's being used in Search, Field Caps, and PIT (this).
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.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.
Approved, but I'd like to have Pawan's approval before merging.
server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java
Outdated
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. Any outstanding comments are minor nitpicks and not blockers. Approving.
Enables _pit endpoint to be cross project. (allowsCrossProject return true)
Disable cross project chaining (by calling indicesOptionsForCrossProjectFanout that sets CrossProjectModeOptions back to default)
Adds project routing parameter in _pit if crossProjectEnabled (query param only)
Disallows projectRouting for _search if in CPS context and pit id is provided.
Missing: Allow UIAM for close pit endpoint.