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

feat: make job deployment asynchronous #296

Merged
merged 32 commits into from
May 10, 2022
Merged

feat: make job deployment asynchronous #296

merged 32 commits into from
May 10, 2022

Conversation

arinda-arif
Copy link
Contributor

@arinda-arif arinda-arif commented Apr 18, 2022

Job deployment requested from Job Refresh is being done asynchronously by this PR.
#181

@coveralls
Copy link

coveralls commented Apr 18, 2022

Pull Request Test Coverage Report for Build 2292221730

  • 202 of 256 (78.91%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 75.239%

Changes Missing Coverage Covered Lines Changed/Added Lines %
api/handler/v1beta1/job_spec.go 30 32 93.75%
job/service.go 9 11 81.82%
ext/scheduler/airflow2/airflow.go 33 39 84.62%
models/job.go 0 6 0.0%
models/progress.go 0 9 0.0%
api/handler/v1beta1/observer.go 0 13 0.0%
job/deploy_manager.go 103 119 86.55%
Totals Coverage Status
Change from base Build 2280236614: 0.09%
Covered Lines: 6223
Relevant Lines: 8271

💛 - Coveralls

@arinda-arif arinda-arif linked an issue Apr 19, 2022 that may be closed by this pull request
@arinda-arif
Copy link
Contributor Author

raystack/proton#132

cmd/job_refresh.go Outdated Show resolved Hide resolved
cmd/job_refresh.go Outdated Show resolved Hide resolved
cmd/job_refresh.go Outdated Show resolved Hide resolved
cmd/job_refresh.go Outdated Show resolved Hide resolved
api/handler/v1beta1/job_spec.go Outdated Show resolved Hide resolved
ext/scheduler/airflow2/airflow.go Outdated Show resolved Hide resolved
go m.spawnDeployer(m.deployer)
}

// wait until all workers are ready
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use work group wait instead of us handling it explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use wait group.

job/deploy_manager.go Outdated Show resolved Hide resolved
job/deploy_manager.go Show resolved Hide resolved
job/deploy_manager.go Show resolved Hide resolved
status VARCHAR(15) NOT NULL,
details JSONB,
created_at TIMESTAMP WITH TIME ZONE NOT NULL,
updated_at TIMESTAMP WITH TIME ZONE
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we expecting this field to be nullable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the updated_at, yes, it can be null if it is just created.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was wondering as gorm has this field marked as not null

UpdatedAt time.Time `gorm:"not null" json:"updated_at"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh just realized it. I can remove the not null from gorm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually seeing from other tables, we are keeping UpdatedAt as NOT NULL, I think the value is filled with when the record is created initially. maybe I can change to follow this (having it as not null), what do you think? @sbchaos

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, let's keep the field as not null as in other tables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated this

job/deploy_manager.go Outdated Show resolved Hide resolved
job/deploy_manager_test.go Outdated Show resolved Hide resolved
job/deploy_manager_test.go Outdated Show resolved Hide resolved
cmd/job_refresh.go Outdated Show resolved Hide resolved
@arinda-arif
Copy link
Contributor Author

@sravankorumilli @sbchaos please help to re-review this. have committed the changes based on what was discussed. thank you 🙂

@sravankorumilli sravankorumilli merged commit 1273506 into main May 10, 2022
@sravankorumilli sravankorumilli deleted the refresh-async branch May 10, 2022 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recompiling & Refreshing of all Jobs to be made asynchronous
4 participants