-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Prism] Connect Web UI cancel requests with backend #31028
[Prism] Connect Web UI cancel requests with backend #31028
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label go. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
sdks/go/pkg/beam/runners/prism/internal/jobservices/management.go
Outdated
Show resolved
Hide resolved
sdks/go/pkg/beam/runners/prism/internal/jobservices/management.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I do have one comment.
@@ -243,8 +243,8 @@ func (s *Server) Run(ctx context.Context, req *jobpb.RunJobRequest) (*jobpb.RunJ | |||
// Otherwise, returns nil if Job does not exist or the Job's existing state as part of the CancelJobResponse. | |||
func (s *Server) Cancel(_ context.Context, req *jobpb.CancelJobRequest) (*jobpb.CancelJobResponse, error) { | |||
s.mu.Lock() | |||
defer s.mu.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deferring unlock isn't a straightforward universal good.
So this locks the whole server's shared state until the Cancel is complete, while it's doing the per job changes. You would at best want to lock the job's state, not serialize everything the server is handling.
The various job state fields should be locked through a job specific lock, which I suppose might be job.streamCond.L. State in particular is an atomic to avoid needing a full job lock for every read. With cancelling that might need to change however.
The Prepare method definitely has some overzealous locking (it's doing a lot of work, but not a lot of mixing with the server side state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it makes sense to me that state is an atomic.Value and thus I didn't need to put a lock on the whole service. I only lock/unlock when acquiring the Job just like all the other methods.
About
This PR addresses #29669 connecting Web UI cancel requests with the backend Job Management server.
Changes
Testing
In addition to
go test ...
, manual testing of this PR was as follows:go run ./go/cmd/prism
go run ./go/examples/large_wordcount --output=/tmp/large_wordcount/out --runner=universal --endpoint=localhost:8073 --environment_type=LOOPBACK
RUNNING
RUNNING
toCANCELLING
CANCELED
CANCELED
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.UpdateCHANGES.md
with noteworthy changes.If this contribution is large, please file an Apache Individual Contributor License Agreement.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.