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

prototype(worker-tasks): Create pending task store abstraction #77658

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

enochtangg
Copy link
Member

No description provided.

@enochtangg enochtangg requested review from a team as code owners September 17, 2024 21:05
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 17, 2024
@enochtangg enochtangg marked this pull request as draft September 17, 2024 21:06
@enochtangg enochtangg added the Do Not Merge Don't merge label Sep 17, 2024
Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

This is too large to review as is. I'd recommend breaking it up, and adding tests to each section as you go

@john-z-yang john-z-yang changed the base branch from master to hackweek-kafkatasks September 17, 2024 21:28
@john-z-yang
Copy link
Member

This is too large to review as is. I'd recommend breaking it up, and adding tests to each section as you go

This is not meant to target prod, it's for prototyping only. Apologies for the confusion!


@dataclass
class PendingTask:
proto_task: PendingTaskProto
Copy link
Member

Choose a reason for hiding this comment

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

👍 Decorating with a 'domain object' is a good path to take. Copying all the values out of the protobuf will be expensive in aggregate.

We might want to add methods/properties to this object that proxy to the proto_task and have the proto_task be a private property.

from dataclasses import dataclass
from enum import Enum

from sentry_protos.hackweek_team_no_celery_pls.v1alpha.pending_task_pb2 import (
Copy link
Member

Choose a reason for hiding this comment

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

We'll have to come up with a better name for this 😄

@@ -224,3 +120,4 @@ def do_upkeep(

def shutdown(self):
self.pool.close()
self.pool.close()
Copy link
Member

Choose a reason for hiding this comment

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

Why twice?

Copy link
Member

Choose a reason for hiding this comment

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

This is a mistake, it's fixed now

@john-z-yang john-z-yang merged commit 51caaea into hackweek-kafkatasks Sep 18, 2024
17 of 43 checks passed
@john-z-yang john-z-yang deleted the pending-task-store branch September 18, 2024 17:35
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Do Not Merge Don't merge Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants