Skip to content

chore(spanner): add multiplexed session flag in executor code #3030

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Apr 23, 2024
Merged

chore(spanner): add multiplexed session flag in executor code #3030

merged 9 commits into from
Apr 23, 2024

Conversation

harshachinta
Copy link
Contributor

@harshachinta harshachinta commented Apr 12, 2024 •
edited
Loading

This PR has the changes to accept a new flag --multiplexed_session_operations_ratio as input argument when running the executor artifact.
Based on the value of multiplexed_session_operations_ratio flag the client will either use regular sessions or multiplexed sessions

  1. If the value is > 0.0, then all the supported operations will use multiplexed sessions.
  2. If the value is == 0.0, then all the operations will use regular sessions

Note: To start with, we are converting this ratio (double) into boolean property. In future we will use this ratio to perform a load splitter operation where some transactions use multiplexed sessions whereas the rest uses regular sessions.

@harshachinta harshachinta requested a review from a team as a code owner April 12, 2024 12:27
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: spanner Issues related to the googleapis/java-spanner API. labels Apr 12, 2024
@harshachinta harshachinta requested a review from arpan14 April 12, 2024 12:27
@harshachinta harshachinta changed the title chore(spanner): add multiplexed session flag for executor code chore(spanner): add multiplexed session flag in executor code Apr 12, 2024
@harshachinta harshachinta requested a review from a team as a code owner April 12, 2024 14:35
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Apr 12, 2024
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Apr 16, 2024
@harshachinta harshachinta added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 16, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 16, 2024
@harshachinta harshachinta requested a review from arpan14 April 16, 2024 14:15
@harshachinta
Copy link
Contributor Author

@gyang-google Can you help review this?

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Apr 16, 2024
@harshachinta harshachinta requested review from arpan14 and removed request for arpan14 April 22, 2024 13:43
arpan14
arpan14 reviewed Apr 22, 2024
View reviewed changes
...loud-spanner-executor/src/main/java/com/google/cloud/executor/spanner/CloudExecutorImpl.java
// multiplexedSessionOperationsRatio from command line is > 0.0
if (multiplexedSessionOperationsRatio > 0.0) {
SessionPoolOptions.Builder sessionPoolOptionsBuilder;
if (request.getAction().getSpannerOptions().hasSessionPoolOptions()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a check like request.getAction().hasSpannerOptions() before doing getSpannerOptions. Else this will throw a NullPointerException.

Copy link
Contributor Author

@harshachinta harshachinta Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The autogenerated methods from the proto always returns a default instance of the class and not null. In this case getAction(), getSpannerOptions etc will never be null. Hence, we are safe from not getting an NullPointerException
Reference:

java-spanner/proto-google-cloud-spanner-executor-v1/src/main/java/com/google/spanner/executor/v1/SpannerAsyncActionRequest.java

Lines 112 to 115 in 6f522dd

public com.google.spanner.executor.v1.SpannerAction getAction() {
return action_ == null
? com.google.spanner.executor.v1.SpannerAction.getDefaultInstance()
: action_;

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, got it.

...loud-spanner-executor/src/main/java/com/google/cloud/executor/spanner/CloudExecutorImpl.java
SpannerOptions.Builder optionsBuilder =
request
.getAction()
.getSpannerOptions()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think getSpannerOptions will throw a NullPointerException since no one is passing it today. Add a handling similar to what you did for SessionPoolOptions above.

Copy link
Contributor Author

@harshachinta harshachinta Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The autogenerated methods from the proto always returns a default instance of the class and not null. In this case getAction(), getSpannerOptions etc will never be null eventhough they are not set in input. Hence, we are safe from not getting an NullPointerException
Reference:

java-spanner/proto-google-cloud-spanner-executor-v1/src/main/java/com/google/spanner/executor/v1/SpannerAsyncActionRequest.java

Lines 112 to 115 in 6f522dd

public com.google.spanner.executor.v1.SpannerAction getAction() {
return action_ == null
? com.google.spanner.executor.v1.SpannerAction.getDefaultInstance()
: action_;

...loud-spanner-executor/src/main/java/com/google/cloud/executor/spanner/CloudExecutorImpl.java
.setSessionPoolOptions(sessionPoolOptionsBuilder);
SpannerAction.Builder actionBuilder =
request.getAction().toBuilder().setSpannerOptions(optionsBuilder);
request = request.toBuilder().setAction(actionBuilder).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we don't have unit tests, can you please link a before/after log object of this request object before and after your change? This is to ensure we are not mis-constructing the entire input object and avoid bugs like above.

Copy link
Contributor Author

@harshachinta harshachinta Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have added a short log here.

Request object Before:
Screenshot 2024-04-23 at 3 19 16 PM

Request Object after adding multiplexed session
Screenshot 2024-04-23 at 3 24 38 PM

google-cloud-spanner-executor/src/main/java/com/google/cloud/executor/spanner/WorkerProxy.java
@@ -95,10 +98,22 @@ public static void main(String[] args) throws Exception {
usePlainTextChannel = commandLine.hasOption(OPTION_USE_PLAIN_TEXT_CHANNEL);
enableGrpcFaultInjector = commandLine.hasOption(OPTION_ENABLE_GRPC_FAULT_INJECTOR);

if (commandLine.hasOption(OPTION_MULTIPLEXED_SESSION_OPERATIONS_RATIO)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you made sure this works when the value is null? Do we have some manual logs to validate that this is working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OPTION_MULTIPLEXED_SESSION_OPERATIONS_RATIO flag value will never be null. The default value passed from systest is 0.0.

google-cloud-spanner-executor/src/main/java/com/google/cloud/executor/spanner/WorkerProxy.java
@@ -95,10 +98,22 @@ public static void main(String[] args) throws Exception {
usePlainTextChannel = commandLine.hasOption(OPTION_USE_PLAIN_TEXT_CHANNEL);
enableGrpcFaultInjector = commandLine.hasOption(OPTION_ENABLE_GRPC_FAULT_INJECTOR);

if (commandLine.hasOption(OPTION_MULTIPLEXED_SESSION_OPERATIONS_RATIO)) {
multiplexedSessionOperationsRatio =
Double.parseDouble(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If value is not null, we can add a validation that it is between 0 and 100. Similar validation is present for ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a validation for multiplexed session ratio flag value to be between 0.0 and 1.0

arpan14
arpan14 approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@arpan14 arpan14 left a comment

Choose a reason for hiding this comment

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

LGTM, please take care of the testing before merging. And if possible, attach the test results.

@harshachinta harshachinta added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 23, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 23, 2024
@harshachinta
Copy link
Contributor Author

harshachinta commented Apr 23, 2024 •
edited
Loading

Tested the code changes by running systests locally. The runs were successful.
https://screenshot.googleplex.com/3AEDfgWqyRm6TkY

Test Driver Link - https://sigma.corp.google.com/job/ir/spanner-test/systest-sriharshach-cloudclientquickmainline.driver/4208880026702?terminated=true

Backend Metrics that show Multiplexed session as enabled - https://screenshot.googleplex.com/966zkSeBPhseGAg

@harshachinta harshachinta merged commit 1d91283 into googleapis:main Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arpan14 arpan14 arpan14 approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@harshachinta @arpan14