-
-
Notifications
You must be signed in to change notification settings - Fork 1
[STREAM-640] Push-Based Broker / Worker POC Protos #160
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest Buf updates on your PR. Results from workflow ci / buf-checks (pull_request).
|
…try/sentry-protos into george/push-broker-worker
|
|
||
| # Generated bindings | ||
| /py/sentry_protos/* | ||
| # /py/sentry_protos/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just so I can use this repository instead of the official package when building Sentry for testing purposes. If I don't include the generated code it doesn't work, but it will be undone in the final version!
| @@ -0,0 +1,20 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package sentry_protos.taskworker.v1; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we keep these in the taskbroker.v1 package? It is likely that these protos will change alongside those ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 👍
| // If fetch_next is provided, receive a new task in the response | ||
| optional FetchNextTask fetch_next_task = 3; | ||
|
|
||
| string address = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of address is this? A comment could help here.
| @@ -0,0 +1 @@ | |||
| __version__ = "0.4.10" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until this point we've not included python generated code in the repository. Instead we generate the python bindings when building a packaged release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yepp, this is temporary so that I can use this repository as the source for the sentry-protos package in sentry.
| // If fetch_next is provided, receive a new task in the response | ||
| optional FetchNextTask fetch_next_task = 3; | ||
|
|
||
| string address = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be optional if you're adding it to the existing version. Otherwise it might break services that aren't sending this address through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I think this is actually unnecessary, so it'll likely be gone in the final version anyways.
| // Update the state of a task with execution results. | ||
| rpc SetTaskStatus(SetTaskStatusRequest) returns (SetTaskStatusResponse) {} | ||
|
|
||
| // Add a worker to the broker's inner worker pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Add a worker to the broker's inner worker pool. | |
| // Register a worker as able to receive tasks from a broker. |
See this PR.