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

Conversation

keith
Copy link
Member

@keith keith commented Jun 24, 2021

Closes #13599

@@ -46,16 +46,6 @@
})
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

@aiuto aiuto removed their request for review July 7, 2021 14:00
@keith keith requested a review from larsrc-google July 8, 2021 00:05
@larsrc-google
Copy link
Contributor

It's fair to remove an experimental flag, but I prefer giving users a bit of notice.

What bothers me about this flag is exactly that there is no way to negotiate with a worker which protocol to use. If a worker implementation changes from one to the other, there's a real risk of protocol mismatch. But then a global flag doesn't help much, since it would also require other workers to support both.

I'm currently doing a refactoring of the graveyards, I'll flip and graveyard this flag afterwards.

@keith keith force-pushed the ks/remove-experimental_worker_allow_json_protocol branch from 0aa7483 to ce9e21b Compare July 26, 2021 21:04
keith added a commit to keith/bazel that referenced this pull request Jul 26, 2021
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.
@keith
Copy link
Member Author

keith commented Jul 26, 2021

Yea workers only being able to support 1 is interesting, but I guess the expectation is users shouldn't need to know which protocol a worker uses anyways?

Either way I submitted #13745 just to do the flip now, I opted not to move it around further given your comment about refactoring the graveyard logic

bazel-io pushed a commit that referenced this pull request Jan 24, 2022
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
@keith keith closed this Jan 29, 2022
@keith keith deleted the ks/remove-experimental_worker_allow_json_protocol branch January 29, 2022 04:48
@aiuto
Copy link
Contributor

aiuto commented Jan 31, 2022

In retrospect, we should be clearer in the description of this kind of PR. It does not remove the feature enabled by experimental_worker_allow_json_protocol. It just removes the flag and makes the feature permanent. When looking at revision history, it will be easier to understand if phrased that way.

@aiuto aiuto changed the title Remove --experimental_worker_allow_json_protocol Remove --experimental_worker_allow_json_protocol flag. Worker protocol over JSON is now a permanent feature. Jan 31, 2022
@keith
Copy link
Member Author

keith commented Jan 31, 2022

Now that this feature is flipped I'm less worried about this flag existing and just doing nothing, there's no reason to turn it off. I assumed at this point we might want to give users a bit just so the flag doesn't start erroring since it was so recently flipped.

@aiuto
Copy link
Contributor

aiuto commented Jan 31, 2022

I'm beginning a quest to remove as many unneeded flags as possible from Bazel over the next year. We just have too many. But, that's just like my opinion.

@keith keith restored the ks/remove-experimental_worker_allow_json_protocol branch January 31, 2022 23:41
@keith
Copy link
Member Author

keith commented Jan 31, 2022

I create #14679

brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Feb 8, 2022
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)
Wyverald pushed a commit that referenced this pull request Feb 9, 2022
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

json persistent workers GA / --experimental_worker_allow_json_protocol removal
3 participants