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

Add functionality to restart always running jobs #715

Merged
merged 6 commits into from
Jul 8, 2021
Merged

Add functionality to restart always running jobs #715

merged 6 commits into from
Jul 8, 2021

Conversation

nfx
Copy link
Contributor

@nfx nfx commented Jul 7, 2021

@nfx nfx linked an issue Jul 7, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #715 (1010a0d) into master (f17e41f) will increase coverage by 0.06%.
The diff coverage is 97.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
+ Coverage   83.49%   83.56%   +0.06%     
==========================================
  Files          87       87              
  Lines        8015     8060      +45     
==========================================
+ Hits         6692     6735      +43     
- Misses        842      843       +1     
- Partials      481      482       +1     
Impacted Files Coverage Δ
compute/model.go 76.31% <ø> (ø)
compute/resource_job.go 88.26% <97.50%> (+2.34%) ⬆️
common/resource.go 65.38% <100.00%> (+0.67%) ⬆️

@nfx nfx requested a review from alexott July 7, 2021 10:42
@nfx nfx marked this pull request as ready for review July 8, 2021 09:38
@nfx nfx added this to the v0.3.6 milestone Jul 8, 2021
@@ -573,7 +573,9 @@ func (j Job) ID() string {

// RunParameters ...
type RunParameters struct {
// TODO: if we add job_id, it can be also a request to RunNow
// a shortcut field to reuse this type for RunNow
JobID int64 `json:"job_id,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be tf:"computed" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this entity is not exposed to TF - just internal comms

@@ -15,13 +17,14 @@ import (

// NewJobsAPI creates JobsAPI instance from provider meta
func NewJobsAPI(ctx context.Context, m interface{}) JobsAPI {
return JobsAPI{m.(*common.DatabricksClient), ctx}
return JobsAPI{m.(*common.DatabricksClient), ctx, 20 * time.Minute}
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to make timeout configurable? I have seen the situations when stopping the stream could take a significant amount of time

Copy link
Contributor Author

@nfx nfx Jul 8, 2021

Choose a reason for hiding this comment

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

@alexott stopping the stream as in cancelling the job? could you check? there's no way to call dataplane apis, as they require WebUI token instead of PAT or AAD.

we can add the same timeout configuration as for clusters/workspaces.

compute/resource_job.go Outdated Show resolved Hide resolved
compute/resource_job.go Show resolved Hide resolved
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

lgtm

@nfx nfx merged commit 54ef068 into master Jul 8, 2021
@nfx nfx deleted the feature/389 branch July 8, 2021 21:42
@nfx nfx mentioned this pull request Jul 14, 2021
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.

[FEATURE] Restart running streaming job to redeploy configuration changes
2 participants