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

Fix warning on startup [no JIRA] #5570

Merged
merged 5 commits into from
Jul 10, 2020
Merged

Fix warning on startup [no JIRA] #5570

merged 5 commits into from
Jul 10, 2020

Conversation

aednichols
Copy link
Collaborator

Fixes runtime warning introduced in #5565

Intentional stowaway: error message improvement for @barkasn

@cjllanwarne cjllanwarne self-requested a review July 9, 2020 20:01
@@ -251,7 +251,7 @@ case class WorkflowExecutionActor(params: WorkflowExecutionActorParams)
// - The job lasted too long (eg PAPI 6 day timeout)
// Treat it like any other non-retryable failure:
case Event(JobAbortedResponse(jobKey), stateData) =>
val cause = new Exception("The job was aborted from outside Cromwell")
val cause = new Exception(s"The compute backend terminated the job. Likely reasons are preemption, running out of disk or memory on the compute instance, or exceeding the backend's maximum job duration.")
Copy link
Contributor

Choose a reason for hiding this comment

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

TOLish I wonder if someone has scripts targeting the current value of this message...

@@ -251,7 +251,7 @@ case class WorkflowExecutionActor(params: WorkflowExecutionActorParams)
// - The job lasted too long (eg PAPI 6 day timeout)
// Treat it like any other non-retryable failure:
case Event(JobAbortedResponse(jobKey), stateData) =>
val cause = new Exception("The job was aborted from outside Cromwell")
val cause = new Exception(s"The compute backend terminated the job. Likely reasons are preemption, running out of disk or memory on the compute instance, or exceeding the backend's maximum job duration.")
Copy link
Contributor

Choose a reason for hiding this comment

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

In HPC environments (or PAPI I suppose if they know the API calls to make), it can also indicate the user stopping their own job manually.

Not sure that's worth calling out as a separate case though, since in those situations they'd presumably know about it...

@aednichols aednichols requested review from mcovarr and cjllanwarne July 9, 2020 21:10
@aednichols
Copy link
Collaborator Author

Re-requesting reviews because I introduced another stowaway in the changelog

### Google Library Upgrade [(#5565)](https://github.com/broadinstitute/cromwell/pull/5565)

Previous versions of Cromwell use Google libraries that have been deprecated and will [stop working in August 2020](https://developers.googleblog.com/2018/03/discontinuing-support-for-json-rpc-and.html). This release adopts updated libraries to ensure continued smooth operation. No user action is required aside from upgrading Cromwell.

Copy link
Contributor

Choose a reason for hiding this comment

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

TOL so we may need to backport these upgrades into Cromwell 0.12 if the global endpoints really are shut down in August...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you check out 51_hotfix and navigate to BatchRequest.java:98 it specifically uses https://www.googleapis.com/batch.

In the execute() function at BatchRequest.java:216 the value of batchUrl is used unmodified and we definitely don't call the setBatchUrl() mutator from Cromwell, sooooo.....

Copy link
Contributor

Choose a reason for hiding this comment

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

"No user action is required aside from upgrading Cromwell. But yeah, you need to upgrade your Cromwell."

@aednichols aednichols merged commit 381a54e into develop Jul 10, 2020
@aednichols aednichols deleted the aen_warning_fix branch July 20, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants