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

Remove --experimental_worker_allow_json_protocol flag. Worker protocol over JSON is now a permanent feature. #13607

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions site/docs/creating-workers.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ would benefit from cross-action caching, you may want to implement your own
persistent worker to perform these actions.

The Bazel server communicates with the worker using `stdin`/`stdout`. It
supports the use of protocol buffers or JSON strings. Support for JSON is
experimental and thus subject to change. It is guarded behind the
`--experimental_worker_allow_json_protocol` flag.
supports the use of protocol buffers or JSON strings.

The worker implementation has two parts:

Expand Down
18 changes: 6 additions & 12 deletions site/docs/persistent-workers.md
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,6 @@ flag makes each worker request use a separate sandbox directory for all its
inputs. Setting up the [sandbox](sandboxing.md) takes some extra time,
especially on macOS, but gives a better correctness guarantee.

You can use the `--experimental_worker_allow_json_protocol` flag to allow
workers to communicate with Bazel through JSON instead of protocol buffers
(protobuf). The worker and the rule that consumes it can then be modified to
support JSON.

The
[`--worker_quit_after_build`](command-line-reference.html#flag--worker_quit_after_build)
flag is mainly useful for debugging and profiling. This flag forces all workers
Expand Down Expand Up @@ -197,13 +192,12 @@ inputs: [
```

The worker receives this on `stdin` in JSON format (because
`requires-worker-protocol` is set to JSON, and
`--experimental_worker_allow_json_protocol` is passed to the build to enable
this option). The worker then performs the action, and sends a JSON-formatted
`WorkResponse` to Bazel on its stdout. Bazel then parses this response and
manually converts it to a `WorkResponse` proto. To communicate
with the associated worker using binary-encoded protobuf instead of JSON,
`requires-worker-protocol` would be set to `proto`, like this:
`requires-worker-protocol` is set to JSON). The worker then performs the
action, and sends a JSON-formatted `WorkResponse` to Bazel on its
stdout. Bazel then parses this response and manually converts it to a
`WorkResponse` proto. To communicate with the associated worker using
binary-encoded protobuf instead of JSON, `requires-worker-protocol`
would be set to `proto`, like this:

```
execution_requirements = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,6 @@ public class WorkerOptions extends OptionsBase {
})
public Void experimentalPersistentJavac;

@Option(
Copy link
Contributor

@larsrc-google larsrc-google Jun 25, 2021

Choose a reason for hiding this comment

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

You need to move the flag to the graveyard, or any Bazel invocation passing that flag will fail. Also, just removing a flag that defaults to false and switching the effective value to true at the same time is a bad idea: Anyone with a setup that doesn't work with the flag set would start getting failures that they can't avoid with a flag any more. So to start with, just flip the default to true.

Copy link
Member Author

Choose a reason for hiding this comment

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

my thought here was that since this was an undocumented flag for an alpha feature removing it would be ok, especially if that's in the 5.x release not the 4.x release. Also flipping the default wouldn't change much AFAICT since workers can only support 1 protocol at once, so flipping it back to false wouldn't be super useful for folks, still happy to make that change if you think we should, but that's why I just deleted it

name = "experimental_worker_allow_json_protocol",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
help =
"Allows workers to use the JSON worker protocol until it is determined to be"
+ " stable.")
public boolean experimentalJsonWorkerProtocol;

/**
* Defines a resource converter for named values in the form [name=]value, where the value is
* {@link ResourceConverter.FLAG_SYNTAX}. If no name is provided (used when setting a default),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,15 +208,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
}
SandboxOutputs outputs = helpers.getOutputs(spawn);

WorkerProtocolFormat protocolFormat = Spawns.getWorkerProtocolFormat(spawn);
if (!workerOptions.experimentalJsonWorkerProtocol) {
if (protocolFormat == WorkerProtocolFormat.JSON) {
throw new IOException(
"Persistent worker protocol format must be set to proto unless"
+ " --experimental_worker_allow_json_protocol is used");
}
}

WorkerKey key =
new WorkerKey(
workerArgs,
Expand All @@ -228,7 +219,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
context.speculating(),
multiplex && Spawns.supportsMultiplexWorkers(spawn),
Spawns.supportsWorkerCancellation(spawn),
protocolFormat);
Spawns.getWorkerProtocolFormat(spawn));

spawnMetrics =
SpawnMetrics.Builder.forWorkerExec()
Expand Down
1 change: 0 additions & 1 deletion src/test/shell/integration/bazel_worker_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ example_worker=$(find $BAZEL_RUNFILES -name ExampleWorker_deploy.jar)

add_to_bazelrc "build -s"
add_to_bazelrc "build --spawn_strategy=worker,standalone"
add_to_bazelrc "build --experimental_worker_allow_json_protocol"
add_to_bazelrc "build --worker_verbose --worker_max_instances=1"
add_to_bazelrc "build --debug_print_action_contexts"
add_to_bazelrc "build --noexperimental_worker_multiplex"
Expand Down