[prism] Support AnyOf in Prism. #33705
Conversation
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| rankA := ranks[a.GetUrn()] | ||
| rankB := ranks[b.GetUrn()] | ||
|
|
||
| // Reverse the comparison so our favourite is at the front |
There was a problem hiding this comment.
Alternatively we could give the ranks golf-style scoring, or pick the last one in the list below. If this is the most idiomatic I'm fine with that.
There was a problem hiding this comment.
The reversal was partly to have an easier selection for the most choice environment: just the one at 0.
I did consider the golf weighting but I'm also making use of the default zero value for anything in the map. So i biased for the higher weight being priority, vs risk a weird priority inversion if someone puts in negative numbers.
I could avoid the negatives with a uint instead...
As for the idiomatic question: it would be less idiomatic to return the "normal" compare number, then reverse the list. Neither really save lines of code (vs documentation). There is the argument that then the code is self documenting, but we don't really care about the order, just that we picked the "favourite".
Finally, we can always change it later.
| @@ -46,16 +47,26 @@ import ( | |||
|
|
|||
| func runEnvironment(ctx context.Context, j *jobservices.Job, env string, wk *worker.W) error { | |||
| logger := j.Logger.With(slog.String("envID", wk.Env)) | |||
| // TODO fix broken abstraction. | |||
| // We're starting a worker pool here, because that's the loopback environment. | |||
There was a problem hiding this comment.
Was this TODO just obsolete? (I'm not seeing a pool started below, changed or not.)
There was a problem hiding this comment.
Yeah, it's mostly referring to the "one environment, one worker" notion. The "pool" for example could be several "w orkers" all in the same environment.
If we ever decide to make prism distributed and scalable, then this code would need to change anyway, so the TODO is moot.
| @@ -83,6 +96,33 @@ func runEnvironment(ctx context.Context, j *jobservices.Job, env string, wk *wor | |||
| } | |||
| } | |||
|
|
|||
| func selectAnyOfEnv(ap *pipepb.AnyOfEnvironmentPayload) *pipepb.Environment { | |||
| // Prefer external, then process, then docker, unknown environments are 0. | |||
| ranks := map[string]int{ | |||
There was a problem hiding this comment.
This doesn't handle nested AnyOf. Do we care?
There was a problem hiding this comment.
I don't believe we do. We could, but I think we shouldn't.
I'd say it's to Beam's benefit to not encourage that sort of thing.
|
Thanks for the review! |
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.