chore(spanner): add multiplexed session flag in executor code#3030
Conversation
product-auto-label
Bot
added
size: s
harshachinta
changed the title
product-auto-label
Bot
added
size: m
product-auto-label
Bot
added
size: s
harshachinta
added
the
owlbot:run
gcf-owl-bot
Bot
removed
the
owlbot:run
|
@gyang-google Can you help review this? |
product-auto-label
Bot
added
size: m
| // multiplexedSessionOperationsRatio from command line is > 0.0 | ||
| if (multiplexedSessionOperationsRatio > 0.0) { | ||
| SessionPoolOptions.Builder sessionPoolOptionsBuilder; | ||
| if (request.getAction().getSpannerOptions().hasSessionPoolOptions()) { |
There was a problem hiding this comment.
Add a check like request.getAction().hasSpannerOptions() before doing getSpannerOptions. Else this will throw a NullPointerException.
There was a problem hiding this comment.
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:
| SpannerOptions.Builder optionsBuilder = | ||
| request | ||
| .getAction() | ||
| .getSpannerOptions() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
| .setSessionPoolOptions(sessionPoolOptionsBuilder); | ||
| SpannerAction.Builder actionBuilder = | ||
| request.getAction().toBuilder().setSpannerOptions(optionsBuilder); | ||
| request = request.toBuilder().setAction(actionBuilder).build(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have added a short log here.
Request object Before:
Request Object after adding multiplexed session
| usePlainTextChannel = commandLine.hasOption(OPTION_USE_PLAIN_TEXT_CHANNEL); | ||
| enableGrpcFaultInjector = commandLine.hasOption(OPTION_ENABLE_GRPC_FAULT_INJECTOR); | ||
|
|
||
| if (commandLine.hasOption(OPTION_MULTIPLEXED_SESSION_OPERATIONS_RATIO)) { |
There was a problem hiding this comment.
Have you made sure this works when the value is null? Do we have some manual logs to validate that this is working?
There was a problem hiding this comment.
OPTION_MULTIPLEXED_SESSION_OPERATIONS_RATIO flag value will never be null. The default value passed from systest is 0.0.
|
|
||
| if (commandLine.hasOption(OPTION_MULTIPLEXED_SESSION_OPERATIONS_RATIO)) { | ||
| multiplexedSessionOperationsRatio = | ||
| Double.parseDouble( |
There was a problem hiding this comment.
Nit: If value is not null, we can add a validation that it is between 0 and 100. Similar validation is present for ports.
There was a problem hiding this comment.
Added a validation for multiplexed session ratio flag value to be between 0.0 and 1.0
arpan14
left a comment
arpan14
left a comment
There was a problem hiding this comment.
LGTM, please take care of the testing before merging. And if possible, attach the test results.
harshachinta
added
the
owlbot:run
gcf-owl-bot
Bot
removed
the
owlbot:run
|
Tested the code changes by running systests locally. The runs were successful. Backend Metrics that show Multiplexed session as enabled - https://screenshot.googleplex.com/966zkSeBPhseGAg |
2 participants
This PR has the changes to accept a new flag
--multiplexed_session_operations_ratioas 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
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.