Skip to content

Remove EqlClient from HLRC#85730

Merged
joegallo merged 10 commits intoelastic:masterfrom
joegallo:remove-eqlclient
Apr 21, 2022
Merged

Remove EqlClient from HLRC#85730
joegallo merged 10 commits intoelastic:masterfrom
joegallo:remove-eqlclient

Conversation

@joegallo
Copy link
Copy Markdown
Contributor

@joegallo joegallo commented Apr 6, 2022

Related to #83423

@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/clients-team (Team:Clients)

@joegallo joegallo changed the title Remove EqlClient Remove EqlClient from HLRC Apr 6, 2022
@joegallo joegallo marked this pull request as draft April 6, 2022 19:22
@joegallo joegallo marked this pull request as ready for review April 6, 2022 20:38
@joegallo joegallo requested a review from costin April 7, 2022 13:10
@joegallo joegallo mentioned this pull request Apr 11, 2022
66 tasks
@costin costin requested review from astefan and removed request for costin April 12, 2022 14:30
@astefan astefan requested a review from luigidellaquila April 15, 2022 08:46
@astefan astefan added the :Analytics/EQL EQL querying label Apr 15, 2022
@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Apr 15, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-ql (Team:QL)

Copy link
Copy Markdown
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

In QL one idea about our integration testing was to eat our own dog food, thus in many places (more evident in SQL) we use our own "clients" to test various scenarios. Same used to happen with EQL, thus the use of the HLRC in tests.
With this PR, the HLRC is removed and replaced with low level json parsing. Is it possible to replace the HLRC usage with the new client usage?

@joegallo
Copy link
Copy Markdown
Contributor Author

@astefan I reached out to @mark-vieira to ask about adding a new-java-client dependency in our tests for that purpose, and he's hesitant for us to do it. @mark-vieira can you elaborate on your reasoning?

@mark-vieira
Copy link
Copy Markdown
Contributor

Right now this isn't simple becuase the new Java client lives in its own repository and therefore we would have to rely on snapshot releases for using it in the Elasticsearch build. Right now the Java client is only published as part of the unified release process, meaning that after version bumps, it can take days to get a new artifact. This would mean any builds relying on that artifact would fail in the meantime.

We'll want the new Java client to move to the new daily releasable artifacts process (like ML) before we introduce any dependency on it.

@luigidellaquila
Copy link
Copy Markdown
Contributor

From the practical point of view, the PR LGTM.
👍 about using the low level REST API for tests (thank you @mark-vieira, the explanation makes definitely sense).

Just out of curiosity: if I got it right, the code of the new API is automatically generated.

elastic/elasticsearch-java@d7451d0

Is there anything we have to know or to take into consideration from the QL side about that? Maybe it has impacts on how we can extend (E)QL capabilities?

Copy link
Copy Markdown
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@astefan
Copy link
Copy Markdown
Contributor

astefan commented Apr 19, 2022

We'll want the new Java client to move to the new daily releasable artifacts process (like ML) before we introduce any dependency on it.

@mark-vieira @joegallo is there an issue I can track about this future move to daily releaseable artifacts? Thanks

@mark-vieira
Copy link
Copy Markdown
Contributor

@mark-vieira @joegallo is there an issue I can track about this future move to daily releaseable artifacts? Thanks

Not that I'm aware of. I think the platform release team will coordinate this with the clients team.

@swallez
Copy link
Copy Markdown
Contributor

swallez commented Apr 19, 2022

We'll want the new Java client to move to the new daily releasable artifacts process (like ML) before we introduce any dependency on it.

@mark-vieira what are "daily releasable artifacts"? Is this different from daily snapshots, which are already produced for the new Java client?

One thing to keep in mind is that the new Java client is code-generated from the API specification. So an API change on the server means updating the API spec and generating the client code. Updating the generated code is currently a manual process, although it could (and should!) be automated.

@joegallo joegallo merged commit 746bd3f into elastic:master Apr 21, 2022
@joegallo joegallo deleted the remove-eqlclient branch April 21, 2022 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/EQL EQL querying >non-issue Team:Clients Meta label for clients team Team:QL (Deprecated) Meta label for query languages team v8.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants