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

workers: revision worker implementation #224

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

workers: revision worker implementation #224

wants to merge 1 commit into from

Conversation

zzzeid
Copy link
Contributor

@zzzeid zzzeid commented Nov 15, 2022 •
edited
Loading

NOTE: original branch name references the wrong bug.

WIP DO NOT MERGE

  • implement base RevisionWorker (bug 1788728)
  • implement Supervisor worker (bug 1835861)
  • implement Processor worker (bug 1835862)
  • add repo.use_revision_worker feature flag (bug 1788732)
  • add main worker flag and capacity/throttle flags
  • add method to parse diff and list affected files
  • add test coverage for revision_worker.py
  • add new start/stop commands to manage workers
  • add new flags to stop workers gracefully (*_WORKER_STOPPED)
  • refactor dependency and stack fetching and parsing using networkx
  • rename old command lando-cli landing-worker to lando-cli start-landing-worker

TODO:

  • detect stack change on page load
  • add tests for new warnings

@zzzeid zzzeid mentioned this pull request Nov 15, 2022
Closed
@zzzeid zzzeid marked this pull request as ready for review November 15, 2022 14:41
@zzzeid zzzeid changed the base branch from main to zeid/bug-1791807-upgrade-cache November 15, 2022 14:45
@zzzeid zzzeid force-pushed the zeid/bug-1744327-refactor-workers branch 3 times, most recently from 61a474e to 306ca99 Compare November 16, 2022 14:16
zzzeid
zzzeid commented Nov 16, 2022
View reviewed changes
landoapi/api/transplants.py Outdated Show resolved Hide resolved
@zzzeid zzzeid mentioned this pull request Nov 22, 2022
Merged
zzzeid
zzzeid commented Nov 24, 2022
View reviewed changes
landoapi/app.py Outdated Show resolved Hide resolved
zzzeid
zzzeid commented Nov 24, 2022
View reviewed changes
landoapi/app.py Outdated Show resolved Hide resolved
zzzeid
zzzeid commented Nov 24, 2022
View reviewed changes
landoapi/app.py Outdated Show resolved Hide resolved
zzzeid
zzzeid commented Nov 24, 2022
View reviewed changes
landoapi/app.py Outdated Show resolved Hide resolved
zzzeid
zzzeid commented Nov 24, 2022
View reviewed changes
landoapi/app.py Outdated Show resolved Hide resolved
zzzeid
zzzeid commented Nov 24, 2022
View reviewed changes
landoapi/models/revisions.py Outdated Show resolved Hide resolved
zzzeid
zzzeid commented Nov 24, 2022
View reviewed changes
landoapi/models/revisions.py Outdated Show resolved Hide resolved
cgsheeh
cgsheeh requested changes Nov 24, 2022
View reviewed changes
landoapi/api/revisions.py Outdated Show resolved Hide resolved
landoapi/api/transplants.py Outdated Show resolved Hide resolved
landoapi/models/landing_job.py Outdated Show resolved Hide resolved
landoapi/models/revisions.py Outdated Show resolved Hide resolved
landoapi/models/revisions.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Outdated
Comment on lines 353 to 320
try:
warnings, errors = self.process(revision)
except Exception as e:
logger.info(f"Exception encountered while processing {revision}")
revision.status = RS.PROBLEM
revision.update_data(error="".join(e.args))
logger.exception(e)
db.session.commit()
continue
Copy link
Member

Choose a reason for hiding this comment

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

I think we should catch a different exception here.

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 will look into this one.

landoapi/workers/revision_worker.py Outdated Show resolved Hide resolved
tests/test_sanitized_commit_messages.py Outdated Show resolved Hide resolved
@zzzeid zzzeid requested a review from cgsheeh December 2, 2022 14:34
cgsheeh
cgsheeh requested changes Dec 14, 2022
View reviewed changes
Copy link
Member

@cgsheeh cgsheeh left a comment

Choose a reason for hiding this comment

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

A few more comments, questions etc.

landoapi/api/transplants.py Outdated Show resolved Hide resolved
landoapi/api/transplants.py Outdated Show resolved Hide resolved
landoapi/api/transplants.py Outdated Show resolved Hide resolved
landoapi/api/transplants.py Outdated Show resolved Hide resolved
landoapi/api/transplants.py Outdated Show resolved Hide resolved
landoapi/cli.py Show resolved Hide resolved
landoapi/models/landing_job.py Outdated Show resolved Hide resolved
landoapi/models/landing_job.py Outdated Show resolved Hide resolved
landoapi/models/revisions.py Outdated Show resolved Hide resolved
landoapi/projects.py Outdated Show resolved Hide resolved
cgsheeh
cgsheeh requested changes Dec 22, 2022
View reviewed changes
Copy link
Member

@cgsheeh cgsheeh left a comment

Choose a reason for hiding this comment

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

Please don't use single letter variable names.

landoapi/stacks.py Outdated Show resolved Hide resolved
landoapi/workers/base.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Outdated
Comment on lines 69 to 72
statuses = statuses or [
"draft",
"needs-review",
"accepted",
"published",
"changes-planned",
]
Copy link
Member

Choose a reason for hiding this comment

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

What is the significance of this list as the default? If you could give this a :sort too that would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It limits how many revisions we are synchronizing based on those that we need. The list will probably be tweaked in the future (e.g., maybe we don't sync drafts, or changes-planned, since those won't be landable anyway.)

landoapi/workers/revision_worker.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Show resolved Hide resolved
landoapi/models/revisions.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Outdated Show resolved Hide resolved
landoapi/workers/revision_worker.py Outdated Show resolved Hide resolved
@zzzeid zzzeid force-pushed the zeid/bug-1791807-upgrade-cache branch from c1be7fe to 47de8d0 Compare February 28, 2023 14:45
@zzzeid zzzeid force-pushed the zeid/bug-1791807-upgrade-cache branch 2 times, most recently from 9fea31a to 295c589 Compare March 13, 2023 12:29
Base automatically changed from zeid/bug-1791807-upgrade-cache to main March 13, 2023 12:44
@zzzeid zzzeid force-pushed the zeid/bug-1744327-refactor-workers branch from bd0c611 to c3ec081 Compare March 22, 2023 01:17
@zzzeid zzzeid marked this pull request as draft March 22, 2023 12:12
zzzeid
zzzeid commented Apr 27, 2023
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
zzzeid
zzzeid commented Apr 27, 2023
View reviewed changes
Dockerfile-dev Outdated Show resolved Hide resolved
@zzzeid zzzeid force-pushed the zeid/bug-1744327-refactor-workers branch 4 times, most recently from fcfd98b to 30b64e9 Compare May 11, 2023 19:02
@zzzeid zzzeid force-pushed the zeid/bug-1825277-landing-job-revisions branch 4 times, most recently from 0c15645 to e093d5f Compare May 17, 2023 19:24
Base automatically changed from zeid/bug-1825277-landing-job-revisions to main May 17, 2023 19:30
@zzzeid zzzeid force-pushed the zeid/bug-1744327-refactor-workers branch 5 times, most recently from 3225339 to 8e73e5a Compare May 24, 2023 19:12
@zzzeid zzzeid force-pushed the zeid/bug-1744327-refactor-workers branch 4 times, most recently from 00ba1c3 to f3ededa Compare May 29, 2023 17:27
@zzzeid zzzeid changed the base branch from main to zeid/bug-1835678-add-lando-revision-to-api May 29, 2023 17:28
@zzzeid zzzeid force-pushed the zeid/bug-1744327-refactor-workers branch 2 times, most recently from 0e58aeb to 18abe99 Compare May 29, 2023 17:45
Base automatically changed from zeid/bug-1835678-add-lando-revision-to-api to main May 29, 2023 20:35
@zzzeid zzzeid force-pushed the zeid/bug-1744327-refactor-workers branch 3 times, most recently from 0fb50f6 to 5240202 Compare May 30, 2023 19:24
zzzeid
zzzeid commented May 30, 2023
View reviewed changes
landoapi/workers/revision_worker.py
for r in revision.predecessors + [revision]:
try:
hgrepo.apply_patch(io.BytesIO(r.patch_bytes))
except Exception as e:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this comment.

@zzzeid
workers: revision worker implementation
dd677b6 
NOTE: original branch name references the wrong bug.

WIP DO NOT MERGE

- implement base RevisionWorker (bug 1788728)
- implement Supervisor worker (bug 1835861)
- implement Processor worker (bug 1835862)
- add repo.use_revision_worker feature flag (bug 1788732)
- add main worker flag and capacity/throttle flags
- add method to parse diff and list affected files <*******
- add test coverage for revision_worker.py
- add new start/stop commands to manage workers
- add new flags to stop workers gracefully (*_WORKER_STOPPED)
- refactor dependency and stack fetching and parsing using networkx <********
- rename old command lando-cli landing-worker to lando-cli start-landing-worker

TODO:
- detect stack change on page load
- add tests for new warnings
@zzzeid zzzeid force-pushed the zeid/bug-1744327-refactor-workers branch from 5240202 to dd677b6 Compare May 31, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgsheeh cgsheeh cgsheeh requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zzzeid @cgsheeh