Skip to content
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

[ML] Improve performance of job exists check #77156

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Sep 10, 2020

The endpoint /api/ml/jobs/jobs_exist does not need to use a regex to check the job ids as the underlying es endpoint accepts wildcarded ids.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Code LGTM

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM with optional nit

results[jobId] = body.count > 0;
} catch (e) {
// if a non-wildcarded job id is supplied, the get jobs endpoint will 404
if (e.body.status !== 404) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: if the caught error isn't returned from ES, but instead is the result of an unexpected runtime error, then there's a possibility that you won't have a body property defined. Something like this might be more robust:

Suggested change
if (e.body.status !== 404) {
if (e.body?.status !== 404) {

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested endpoint and LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jgowdyelastic jgowdyelastic merged commit ae9a9c2 into elastic:master Sep 10, 2020
@jgowdyelastic jgowdyelastic deleted the improve-perfornamce-of-job-exists-check branch September 10, 2020 15:06
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Sep 10, 2020
* [ML] Improve performance of job exists check

* adding tests

* possible undefined error body
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Sep 10, 2020
* [ML] Improve performance of job exists check

* adding tests

* possible undefined error body
# Conflicts:
#	x-pack/plugins/ml/server/models/job_service/jobs.ts
jgowdyelastic added a commit that referenced this pull request Sep 10, 2020
* [ML] Improve performance of job exists check (#77156)

* [ML] Improve performance of job exists check

* adding tests

* possible undefined error body
# Conflicts:
#	x-pack/plugins/ml/server/models/job_service/jobs.ts

* fixing type error
jgowdyelastic added a commit that referenced this pull request Sep 10, 2020
* [ML] Improve performance of job exists check

* adding tests

* possible undefined error body
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants