Skip to content

Conversation

@imotov
Copy link
Contributor

@imotov imotov commented Jun 4, 2020

The async task task maintenance service is used by both async search plugin
as well as EQL plugin. So it needs to reside in the core.

Relates to #49638

The async task task maintenance service is used by both async search plugin
as well as EQL plugin. So it needs to reside in the core.

Relates to elastic#49638
@imotov imotov added >enhancement :Search/Search Search-related issues that do not fall into other categories :Analytics/EQL EQL querying labels Jun 4, 2020
@imotov imotov requested review from costin and jimczi June 4, 2020 20:56
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jun 4, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (:Query Languages/EQL)

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jun 4, 2020
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left one comment

components.add(getLicenseService());
components.add(getLicenseState());

if (DiscoveryNode.isDataNode(environment.settings())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me wonder if we should make the index a system index first ? This way, we could start the maintenance service in the system index plugin. I don't think it will cause issue to move the index into a system index since we have all the protection in place with the async search user ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to convert AsyncSearch plugin into SystemIndexPlugin or should I create a separate "Async" plugin for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can have a dedicated plugin since the goal is to start the service for all users of the index ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

A PR that removes code while preserving functionality? 👍

@imotov
Copy link
Contributor Author

imotov commented Jun 10, 2020

I added registration of system index and I also tried to move move some pieces into async plugin but it causes quite a bit of dependencies issues. We can try to move things around, but it seems to be a bigger effort and I would prefer to merge this branch first, since its prolonged maintenance takes time and increases chances of missing changes while resolving conflicts.

@jimczi
Copy link
Contributor

jimczi commented Jun 10, 2020

I added registration of system index and I also tried to move move some pieces into async plugin but it causes quite a bit of dependencies issues.

I think we can restrain the async plugin to declare the system index and to start the maintenance service. It shouldn't be more than that imo. This way we can avoid starting the service in XPackPlugin and the maintenance service code can stay in x-pack core. WDYT ?

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM, I'd rename AsyncPlugin into AsyncIndexPlugin or something along those lines but no strong feeling here. Thanks for iterating on this!

@imotov
Copy link
Contributor Author

imotov commented Jun 11, 2020

@elasticsearchmachine run elasticsearch-ci/2

@imotov imotov merged commit 509c674 into elastic:feature/async-eql Jun 11, 2020
@imotov imotov deleted the move-async-cleanup-service branch June 12, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/EQL EQL querying >enhancement :Search/Search Search-related issues that do not fall into other categories Team:QL (Deprecated) Meta label for query languages team Team:Search Meta label for search team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants