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

Multiplex Workers documentation #10108

Closed
wants to merge 2 commits into from

Conversation

borkaehw
Copy link
Contributor

A documentation for Multiplex Workers. See the original PR for details. #6857

Fix: #2832

@borkaehw borkaehw requested a review from aiuto as a code owner October 25, 2019 19:45
@jin jin added the team-Local-Exec Issues and PRs for the Execution (Local) team label Oct 25, 2019
@aiuto aiuto requested review from philwo and removed request for aiuto October 29, 2019 13:46
@aiuto
Copy link
Contributor

aiuto commented Oct 29, 2019

I'm turfing this to @philwo because he should be a better reviewer for this than I would be.

Copy link
Member

@philwo philwo left a comment

Choose a reason for hiding this comment

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

Thank you!


## A Guide to Write a Multiplex-Compatible Ruleset

The rule's worker process should be multi-threaded to take advantage of Multiplex Workers. Protobuf allows a ruleset to parse a single request even though there might be multiple requests piling up in the stream. Whenever the worker process parses a request from the stream, it should handle the request in a new thread. Since different thread could complete and write to the stream at the same time, the ruleset needs to make sure the responses are written atomically (i.e. messages don't overlap). Responses must contain the `request_id` of the request they're handling.
Copy link
Member

Choose a reason for hiding this comment

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

I would replace "the ruleset needs to make sure" with "the worker process needs to make sure" (because this locking as to be implemented in the worker process, not in the Starlark rules).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@borkaehw
Copy link
Contributor Author

@philwo friendly ping here. Would you be able to take another look?

@philwo
Copy link
Member

philwo commented Nov 12, 2019

Sorry @borkaehw! Merging this now.

@bazel-io bazel-io closed this in ac04cd9 Nov 12, 2019
@borkaehw
Copy link
Contributor Author

Thanks @philwo. How can we wire it up so people can read it in webpage form?

@borkaehw borkaehw deleted the multiplexer-doc branch November 12, 2019 17:06
@borkaehw
Copy link
Contributor Author

Never mind, it seems to be automatic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Local-Exec Issues and PRs for the Execution (Local) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiplex persistent worker protocol
5 participants