-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Add multi-project test for reindex cancellation #140505
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
base: main
Are you sure you want to change the base?
Conversation
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
PeteGillinElastic
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, LGTM, with a couple of comments. The nits you can ignore if you like, but hopefully the suggested base class change will work and simplify this.
.../src/javaRestTest/java/org/elasticsearch/reindex/management/ReindexCancelMultiProjectIT.java
Outdated
Show resolved
Hide resolved
.../src/javaRestTest/java/org/elasticsearch/reindex/management/ReindexCancelMultiProjectIT.java
Outdated
Show resolved
Hide resolved
|
|
||
| import static org.hamcrest.Matchers.equalTo; | ||
|
|
||
| public class ReindexCancelMultiProjectIT extends ESRestTestCase { |
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 should extend MultiProjectRestTestCase instead. This will save you a bunch of work: you don't need to override shouldConfigureProjects, you don't need removeNonDefaultProjects, and you can use setRequestProjectId as a helper instead of your method of the same name. (I don't think I'm missing any reason why this wouldn't work?)
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.
great point!
i already tried this and found this:
MultiProjectRestTestCaseexists intest/external-modules/multi-project/src/javaRestTest/...- all classes that extend that class exist in same "place":
~/repos/elasticsearch $ rg -l 'extends MultiProjectRestTestCase'
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/action/index/IndexDocumentMultiProjectIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/action/admin/indices/stats/IndicesStatsMultiProjectIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/action/admin/indices/SearchMultiProjectIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/action/admin/indices/IndexMultiProjectCRUDIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/action/admin/cluster/stats/ClusterStatsMultiProjectIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/action/admin/cluster/node/stats/NodesStatsMultiProjectIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/multiproject/MultiProjectRestartIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/multiproject/action/ProjectCrudActionIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/multiproject/MultiProjectClusterStateActionIT.java
test/external-modules/multi-project/src/javaRestTest/java/org/elasticsearch/xpack/security/SecurityRolesMultiProjectIT.javaso (as a person still wrapping their mind around gradle/ES deps), commentating:
- all other multi-project tests i can find (ctrl-f
.setting("test.multi_project.enabled", "true")) just use EsRestTestCase - importing other modules javaRestTests sounds nonsensical (also can't find an example, but maybe need guidance), and we'd have to export them somehow
- could do something around putting this into somewhere shared (sounds more sensible), sounds a bit out of scope for me (but i could be being lazy here)
pushed and going to set auto-merge before i sign off and can adjust in a follow-up PR if we need changes here
thoughts? 🙏
samxbr
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, thanks!
.../src/javaRestTest/java/org/elasticsearch/reindex/management/ReindexCancelMultiProjectIT.java
Outdated
Show resolved
Hide resolved
…i-project-tests * upstream/main: (23 commits) Fix `testAckListenerReceivesNacksIfPublicationTimesOut` (elastic#140514) Reduce priority of clear-cache tasks (elastic#139685) Add docs and tests about `StreamOutput` to memory (elastic#140365) ES|QL - dense_vector support for COUNT, PRESENT, ABSENT aggregator functions (elastic#139914) Add release notes for v9.2.4 release (elastic#140487) Add release notes for v9.1.10 release (elastic#140488) Add conncectors release notes for 9.1.10, 9.2.4 (elastic#140499) Add parameter support in PromQL query durations (elastic#139873) Improve testing of STS credentials reloading (elastic#140114) Fix zstd native binary publishing script to support newer versions (elastic#140485) Add FlattenedFieldBinaryVsSortedSetDocValuesSyntheticSourceIT (elastic#140489) Store fallback match only text fields in binary doc values (elastic#140189) [DiskBBQ] Use the new merge executor for intra-merge parallelism (elastic#139942) ESQL: introduce support for mapping-unavailable fields (elastic#140463) Add ESNextOSQVectorsScorerTests (elastic#140436) Disable high cardinality tests on release builds (elastic#140503) ESQL: TRange timezone support (elastic#139911) Directly compressing `StreamOutput` (elastic#140502) ES|QL - fix dense vector enrich bug (elastic#139774) Use CrossProjectModeDecider in RemoteClusterService (elastic#140481) ...
POST _reindex/{taskId}/_cancelAPI #139211reindex-management/qatests modulesuper.match()in previous PR that checks project-ownership inTransportTasksProjectAction