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

restore jobs with file #654

Merged

Conversation

vision9527
Copy link
Contributor

restore jobs with file

@vision9527
Copy link
Contributor Author

@Victorcoder
I have removed download api and squash to one commit.

@vision9527 vision9527 force-pushed the feature/restore_job_with_file branch from 8ff5c16 to ad58bed Compare December 9, 2019 09:07
@vision9527 vision9527 force-pushed the feature/restore_job_with_file branch from e7d4c77 to d225497 Compare February 7, 2020 13:10
@vcastellm
Copy link
Member

Thanks for your work @vision9527 are you still interested in this?

@vision9527
Copy link
Contributor Author

Thanks for your work @vision9527 are you still interested in this?

I'm just in this case. Respect your design philosophy. It's up to you.

Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Some suggestions and rebase with master please

dkron/api.go Outdated Show resolved Hide resolved
dkron/api.go Outdated Show resolved Hide resolved
@vcastellm
Copy link
Member

Also modify the swagger.yml spec please.

@vcastellm
Copy link
Member

@vision9527 also you will need to fix conflicts

@vision9527
Copy link
Contributor Author

@Victorcoder done

dkron/api_test.go Outdated Show resolved Hide resolved
dkron/job.go Outdated Show resolved Hide resolved
@vcastellm
Copy link
Member

@vision9527 could you write the rationale for creating a job tree. I see some test jobs without dependent jobs. In theory if the jobs were exported with the /jobs endpoint, importing them one by one should end up with a correct structure without having to elaborate such a complex parsing.

@vision9527 vision9527 force-pushed the feature/restore_job_with_file branch from 76e2309 to 78b0a74 Compare March 18, 2020 14:29
@vision9527
Copy link
Contributor Author

vision9527 commented Mar 18, 2020

@vision9527 could you write the rationale for creating a job tree. I see some test jobs without dependent jobs. In theory if the jobs were exported with the /jobs endpoint, importing them one by one should end up with a correct structure without having to elaborate such a complex parsing.

Oh, it's my mistake. I change the test jobs code and the endpoint is correct. You can review now. The job tree that has parent job and dependent job can be created with a correct structure. The key is to deal with parent job and dependent job in the endpoint.

@vcastellm
Copy link
Member

@vision9527 Shouldn't the enpoint in this comment #654 (comment) be just restore

I will ask it in another way, what will happen if we just do:

// file: api.go
...
var jobs []*Job
err = json.Unmarshal(data, &jobs)

for _, job := range jobs {
  err := a.GRPCClient.SetJob(job)
}
...

@vision9527 vision9527 force-pushed the feature/restore_job_with_file branch 2 times, most recently from 8d51938 to 312e51f Compare March 22, 2020 14:03
@vision9527
Copy link
Contributor Author

@vision9527 Shouldn't the enpoint in this comment #654 (comment) be just restore

I will ask it in another way, what will happen if we just do:

// file: api.go
...
var jobs []*Job
err = json.Unmarshal(data, &jobs)

for _, job := range jobs {
  err := a.GRPCClient.SetJob(job)
}
...

image
Abort if parent not found before committing job to the store.
It can't be for sure that the create job sequence is right.
Job tree is a array in which parent job is in the front of the dependent jobs, so it can be sure when the dependent job will be created that parent job is exist.

@vision9527
Copy link
Contributor Author

@Victorcoder Please review, bro.

@vcastellm
Copy link
Member

Many changes in master, please fix conflicts or rebase, and we should be good to go, thanks!

@vision9527 vision9527 force-pushed the feature/restore_job_with_file branch from 312e51f to b04fecf Compare April 7, 2020 15:41
@vision9527
Copy link
Contributor Author

Many changes in master, please fix conflicts or rebase, and we should be good to go, thanks!

Done.

Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

After reviewing this again, I left some comments

dkron/store.go Outdated Show resolved Hide resolved
dkron/store.go Outdated Show resolved Hide resolved
@vision9527 vision9527 force-pushed the feature/restore_job_with_file branch from 672b1b8 to f727cfa Compare April 13, 2020 14:46
@vision9527 vision9527 force-pushed the feature/restore_job_with_file branch from f727cfa to 4348306 Compare April 13, 2020 14:51
Copy link
Member

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@vcastellm vcastellm merged commit 51caea0 into distribworks:master Apr 13, 2020
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.

2 participants