chore(spanner): Add changes in Spanner executor for testing end to end tracing #10450
+304
−53
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…into systest-otel-context
…into systest-changes
…into e2e-trace-optin
…into systest-changes
…into systest-changes
harshachinta
reviewed
Jul 4, 2024
| @@ -110,11 +140,20 @@ func (h *CloudStreamHandler) startHandlingRequest(ctx context.Context, req *exec | |||
| } | |||
|
|
|||
| // Get a new action handler based on the input action. | |||
| actionHandler, err := h.newActionHandler(inputAction, outcomeSender) | |||
| actionType, actionHandler, err := h.newActionHandler(inputAction, outcomeSender) | |||
There was a problem hiding this comment.
actionType can be derived directly from inputAction. Passing it from newActionHandler is redundant and is hard to maintain.
My suggestion would be,
Create a new function and use it where required.
func getActionType(inputAction *executorpb.SpannerAction) string {
// GET THE ACTION TYPE
action := fmt.Sprintf("%T", inputAction.GetAction())
// Here action will have output as `*executorpb.SpannerAction_Query`. Apply string processing to convert into required format
}
harshachinta
reviewed
Jul 4, 2024
| // newActionHandler instantiates an actionHandler for executing the given action. | ||
| func (h *CloudStreamHandler) newActionHandler(action *executorpb.SpannerAction, outcomeSender *outputstream.OutcomeSender) (cloudActionHandler, error) { | ||
| func (h *CloudStreamHandler) newActionHandler(action *executorpb.SpannerAction, outcomeSender *outputstream.OutcomeSender) (string, cloudActionHandler, error) { |
There was a problem hiding this comment.
Lets not make this change and instead use the suggestion above.
harshachinta
reviewed
Jul 4, 2024
| return h.OutcomeSender.FinishWithError(err) | ||
| } | ||
| // Create a trace client to read the traces from Cloud Trace. | ||
| traceClient, err := trace.NewClient(ctx, h.TraceClientOptions...) |
There was a problem hiding this comment.
Looks like the traceClient is independent of the type of action and can be initialized just once at the beginning of the Execute() method - here.
Remove it from StartBatchTxnHandler and StartTxnHandler.
harshachinta
reviewed
Jul 4, 2024
| FlowContext *ExecutionFlowContext | ||
| OutcomeSender *outputstream.OutcomeSender | ||
| Options []option.ClientOption | ||
| TraceClientOptions []option.ClientOption |
There was a problem hiding this comment.
This is not needed and can be removed with https://github.com/googleapis/google-cloud-go/pull/10450/files#r1665520383
harshachinta
requested changes
Jul 4, 2024
…into systest-changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds changes in spanner executor code required for testing end to end tracing feature.