-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Flip --experimental_worker_allow_json_protocol #13745
Flip --experimental_worker_allow_json_protocol #13745
Conversation
Based on the discussion here bazelbuild#13607 we want to flip this and leave it around temporarily, but there are also some refactorings coming around how to do that. This flips the value now so it can be used without the flag sooner in rolling releases.
FYI: This change is correct, but I first have to make some internal adjustments. We have some cases where Blaze and workers are released out of sync, so we can't just flip this flag internally, because we can't tell the workers which one to use. |
friendly ping. Can we move this along. It seems mergable. |
Now that the ndjson change merged I assume the blocking changes are internal? Is that correct @larsrc-google ? |
@Wyverald can we cherry pick this for the next 5.x release? |
We shouldn't flip incompatible flags in the middle of an LTS release. This has to wait until 6.x. |
@Wyverald this isn't a breaking change though, since workers can only be one or the other, so I think this is purely additive? |
Ah, I see -- so flipping this flag only enables an extra feature that was already checked in? Then it indeed seems fine. |
Yep, it was just a warning so that the API could be safely changed and users shouldn't be surprised because they had to pass the flag |
Just affirming that this just enables a feature that people have to
willingly invoke.
It should have defaulted to true months ago.
…On Mon, Jan 24, 2022 at 12:30 PM Keith Smiley ***@***.***> wrote:
Yep, it was just a warning so that the API could be safely changed and
users shouldn't be surprised because they had to pass the flag
—
Reply to this email directly, view it on GitHub
<#13745 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXHHHFV36DCIKGI6GTREATUXWECDANCNFSM5BA4J35A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID:
***@***.***>
|
@bazel-io fork 5.1 |
Based on the discussion here bazelbuild#13607 we want to flip this and leave it around temporarily, but there are also some refactorings coming around how to do that. This flips the value now so it can be used without the flag sooner in rolling releases. Closes bazelbuild#13745. PiperOrigin-RevId: 423831532 (cherry picked from commit 9e16a64)
Based on the discussion here #13607 we want to flip this and leave it around temporarily, but there are also some refactorings coming around how to do that. This flips the value now so it can be used without the flag sooner in rolling releases. Closes #13745. PiperOrigin-RevId: 423831532 (cherry picked from commit 9e16a64) Co-authored-by: Keith Smiley <keithbsmiley@gmail.com>
Based on the discussion here
#13607 we want to flip this and
leave it around temporarily, but there are also some refactorings coming
around how to do that. This flips the value now so it can be used
without the flag sooner in rolling releases.