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.

Sign up for GitHub

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

Jump to bottom

Update Rollup Jobs API to accommodate change to ES that allows starting a started job and stopping a stopped job. #38168

Merged
merged 4 commits into from
Jun 10, 2019
Merged

Update Rollup Jobs API to accommodate change to ES that allows starting a started job and stopping a stopped job. #38168

merged 4 commits into from
Jun 10, 2019

Conversation

cjcenizal
Copy link
Contributor

Fixes #25861. Corresponds to elastic/elasticsearch#41502.

To test:

  1. Create two rollup jobs.
  2. Start one.
  3. Select both of them in the table and bulk start them. You shouldn't see any error toasts.
  4. Now stop one of them.
  5. Select both of them in the table and bulk stop them. You shouldn't see any error toasts.

Release note

The UI will no longer display an error if you try to start a started Rollup Job, or stop a stopped one.

@cjcenizal cjcenizal added release_note:fix v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Rollups labels Jun 5, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui

cjcenizal
cjcenizal commented Jun 5, 2019
View reviewed changes
x-pack/plugins/rollup/server/routes/api/jobs.js
@@ -144,7 +135,7 @@ export function registerJobsRoute(server) {
.then(() => ({ success: true }));
} catch(err) {
// There is an issue opened on ES to handle the following error correctly
// https://github.com/elastic/elasticsearch/issues/39845
// https://github.com/elastic/elasticsearch/issues/42908
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to create a new issue here because it looks like the old one didn't capture this case.

@cjcenizal
Copy link
Contributor Author

CC @jgowdyelastic

@cjcenizal cjcenizal requested a review from jen-huang June 5, 2019 23:07
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal
Copy link
Contributor Author

@alisonelizabeth BTW if you test this locally you need to use a build of the latest master on ES. It looks like the daily builds are old, so yarn es snapshot will cause errors in the UI. Likewise, running the tests locally will fail because the functional test server depends on the daily build.

alisonelizabeth
alisonelizabeth approved these changes Jun 6, 2019
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal cjcenizal force-pushed the chore/rollup-start-response branch from fce9b09 to 5001a00 Compare June 10, 2019 16:00
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cjcenizal cjcenizal merged commit 63804cb into elastic:master Jun 10, 2019
@cjcenizal cjcenizal deleted the chore/rollup-start-response branch June 10, 2019 17:07
@cjcenizal cjcenizal mentioned this pull request Jun 10, 2019
Closed
@alisonelizabeth alisonelizabeth mentioned this pull request Feb 7, 2020
Merged
@timroes timroes added the v7.7.0 label Apr 26, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 38168 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 28, 2021
@cjcenizal cjcenizal removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 28, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 38168 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 29, 2021
@cjcenizal cjcenizal added the backport:skip This commit does not require backporting label Apr 29, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

@jen-huang jen-huang Awaiting requested review from jen-huang

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Rollups release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Rollups] If you try to start a rollup job again Kibana displays 500 internal server error
5 participants
@cjcenizal @elasticmachine @kibanamachine @alisonelizabeth @timroes