lostluck commented on code in PR #31028:
URL: https://github.com/apache/beam/pull/31028#discussion_r1581594602
##########
sdks/go/pkg/beam/runners/prism/internal/jobservices/management.go:
##########
@@ -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()
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
