Make downsampling project-aware #124000
Make downsampling project-aware #124000
Conversation
Allows downsampling to work on multiple projects.
nielsbauman
added
>non-issue
:StorageEngine/Downsampling
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
LGTM
I think at some points downsample tests need to be added that run in a multi project test cluster?
| '^security/authz/14_cat_indices/*', | ||
| '^security/authz/14_cat_indices/Test explicit request while multiple opened/*', | ||
| '^security/authz/60_resolve_index/*', | ||
| '^security/authz/80_downsample/*', |
There was a problem hiding this comment.
Was this disabled as part of merging in multi project branch?
There was a problem hiding this comment.
This blacklist/mute list is exclusively used for the "multi-project x-pack YAML test suite". This test suite simply runs the main x-pack YAML test suite, but with multiple projects added to the cluster and the REST client includes one of those projects (the active project) in the header. Therefore, this list is basically just a checklist of features that have not yet been made project-aware. Since this PR makes downsampling project-aware, we can remove it from the list and include it in the multi-project tests.
This test suite will change over time; we'll probably move it to the serverless repo at some point.
ywangd
left a comment
ywangd
left a comment
There was a problem hiding this comment.
LGTM
I have a suggestion for handling project removal which I'd appreciate it to be addressed. Thanks!
server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/persistent/PersistentTasksService.java
Outdated
Show resolved
Hide resolved
| listener.onResponse(PersistentTasksCustomMetadata.getTaskWithId(state.metadata().getProject(projectId), taskId)); | ||
| final var project = state.metadata().projects().get(projectId); | ||
| final PersistentTask<P> task = project == null ? null : PersistentTasksCustomMetadata.getTaskWithId(project, taskId); | ||
| listener.onResponse(task); |
There was a problem hiding this comment.
@ywangd I had to make some changes here to allow the extraction of the task variable. I had to explicitly type the predicate and listener, otherwise the compiler would complain. I think these changes make sense, but I'll let you have another look before I merge.
There was a problem hiding this comment.
If we don't have the separate task variable, we would not need the explicit type parameter changes. i.e. the following works
listener.onResponse(project == null ? null : PersistentTasksCustomMetadata.getTaskWithId(project, taskId));This is already the case with existing code, i.e. the current code does not compile if you have
final var task = PersistentTasksCustomMetadata.getTaskWithId(state, taskId);
listener.onResponse(task); // <--- compilation errorThe wildcard capture of listener.onReponse does not work with other explicit type or another wildcard capture. It needs to infer it by itself.
So I think we can just inline the variable and no need for the type parameter changes.
There was a problem hiding this comment.
to allow the extraction of the task variable
If your goal is to add the separate task variable, the type parameter changes make sense. I don't feel the task variable is necessary. But I don't have strong opinion either.
There was a problem hiding this comment.
I did have a slight preference for the task variable. That combined with the fact that I think having the explicit type parameter makes sense here anyway, resulted in me doing the change this way. In multiple places, we already get (and cast) the state or params from the persistent task passed to the listener, so most usages already depend on the task being a specific instance. That said, I'm fine to revert the changes and inline the variable to reduce the changes of this commit a bit (i.e. keep the scope smaller).
There was a problem hiding this comment.
I see you opened #124180 with the project deletion checks. I'll just wait for you to merge that one and then merge this one. Then I don't have to add the project deletion checks in this PR.
There was a problem hiding this comment.
They are different methods. I think there is no need to for any order between merging the two PRs.
Allows downsampling to work on multiple projects.
Allows downsampling to work on multiple projects.
5 participants
Allows downsampling to work on multiple projects.