Skip to content

Conversation

@bergsieker
Copy link
Contributor

The existing ExecuteAndWait function will rarely actually call
WaitExecution when used against a live server. This change adds the
ability to terminate the Execute call early and switch to WaitExecution
immediately so that it's possible for service implementors to test
WaitExecution.

Retrying based on an operation error is always incorrect, although
mostly harmless. Once an operation has an error, it is marked as Done
and should never be unmarked, so retries will simply fetch the same
operation (with the same error) again.
The existing ExecuteAndWait function will rarely actually call
WaitExecution when used against a live server. This change adds the
ability to terminate the Execute call early and switch to WaitExecution
immediately so that it's possible for service implementors to test
WaitExecution.
}
}
if returnEarly {
return status.Error(codes.Internal, "fake error to for wait call")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "(...) error to for wait (...)", shouldn't it be, "(...) error to trigger the wait (...)"

// ForceEarlyWaitCalls specifies whether ExecuteAndWait should call WaitExecution immedately
// after receiving an Operation from Execute. It is primarily useful for testing
// WaitExecution.
ForceEarlyWaitCalls bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not inheretly opposed to this... But I think the most appropriate place for this behavior would be on the fake server: https://github.com/bazelbuild/remote-apis-sdks/blob/af1232ee0d79920ab8390fb855f95dca18a69cf0/go/pkg/fakes/exec.go. Wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants