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 #14679

Conversation

keith
Copy link
Member

@keith keith commented Jan 31, 2022

This flag has been flipped, and unlike other experimental flags there is no reason to disable it because workers can only support a single format at once, so if they rely on this, you just wouldn't be able to build.

Closes #13599

@keith
Copy link
Member Author

keith commented Jan 31, 2022

cc @aiuto

@keith
Copy link
Member Author

keith commented Jan 31, 2022

Enabling this feature by default should be cherry picked into 5.x, so if we merge this now and leave until 6.x for folks to remove this flag from their configs, that seems safe

@aiuto aiuto requested review from larsrc-google and removed request for jin, floriographygoth and aiuto February 1, 2022 04:08
@gregestren gregestren removed their request for review February 1, 2022 21:00
@gregestren gregestren added the team-Performance Issues for Performance teams label Feb 1, 2022
@keith keith requested a review from fweikert as a code owner February 18, 2022 09:35
@fweikert
Copy link
Member

I'm not that familiar with workers, so I'd like to get an expert review from @larsrc-google

@aiuto aiuto requested review from susinmotion and removed request for fweikert March 25, 2022 18:26
@aiuto
Copy link
Contributor

aiuto commented Mar 25, 2022

My 2 cents:
The feature works and is permanent. We don't need the experimental flag to turn it on, since users select the protocol through other means. So, this CL make sense, AND should be backported to 5.x.
The only think I am not sure of is the one line change of of the protocol format selection.

@susinmotion
Copy link
Contributor

I defer to Lars's expert opinion :) We should flip the flag internally before removing it, but I support doing both of those things.

site/docs/persistent-workers.md Outdated Show resolved Hide resolved
site/docs/persistent-workers.md Outdated Show resolved Hide resolved
@larsrc-google
Copy link
Contributor

@susinmotion The flag is already flipped internally and doesn't seem to be causing any harm. So we can indeed remove this. I don't see any reason to backport it, though.

@keith keith force-pushed the ks/remove-experimental_worker_allow_json_protocol branch from 54ac930 to a920195 Compare April 5, 2022 22:12
@keith
Copy link
Member Author

keith commented Apr 5, 2022

updated the docs

mattrobmattrob added a commit to bazel-ios/rules_ios that referenced this pull request Nov 28, 2022
Port bazelbuild/bazel#14679 into `rules_ios` to prepare for Bazel 6.0.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Performance Issues for Performance teams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

json persistent workers GA / --experimental_worker_allow_json_protocol removal
6 participants