-
Notifications
You must be signed in to change notification settings - Fork 39
Fix: LWT routing enhancements to utilize LBP #784
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.
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
base: scylla-4.x
Are you sure you want to change the base?
Fix: LWT routing enhancements to utilize LBP #784
Conversation
core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java
Outdated
Show resolved
Hide resolved
c4bdac5 to
5bff3d3
Compare
f02d437 to
3c7f0db
Compare
3c7f0db to
5d8ec44
Compare
core/src/main/java/com/datastax/oss/driver/internal/core/cql/DefaultBoundStatement.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/RequestRoutingMethod.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/session/Request.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/RequestRoutingMethod.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/cql/DefaultBatchStatement.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/cql/DefaultBoundStatement.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/cql/DefaultSimpleStatement.java
Outdated
Show resolved
Hide resolved
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Outdated
Show resolved
Hide resolved
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Show resolved
Hide resolved
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Outdated
Show resolved
Hide resolved
c74510a to
5f6c11d
Compare
What's LBP? |
LoadBalancingPolicy |
059281b to
a4f26e1
Compare
dkropachev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last ask
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Outdated
Show resolved
Hide resolved
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java
Outdated
Show resolved
Hide resolved
- Added RequestRoutingType and RequestRoutingMethod enums to define request routing strategies. - Updated DefaultLoadBalancingPolicy to consider request routing type for replica selection, especially for LWT requests. - Updated various graph statement classes (BytecodeGraphStatement, DefaultBatchGraphStatement, DefaultFluentGraphStatement, DefaultScriptGraphStatement) to implement getRequestRoutingType method. - Modified BatchStatementBuilder to set request routing type based on LWT status. - Enhanced DefaultBatchStatement, DefaultBoundStatement, DefaultPrepareRequest, and DefaultSimpleStatement to include routing type in constructors and methods. - Added logic to avoid slow replicas based on health checks and in-flight requests.
6a26ea9 to
8456130
Compare
8456130 to
b2df37b
Compare
core/src/main/java/com/datastax/oss/driver/api/core/cql/StatementBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/cql/StatementBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/cql/DefaultBatchStatement.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/cql/DefaultBatchStatement.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/cql/DefaultBoundStatement.java
Outdated
Show resolved
Hide resolved
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Outdated
Show resolved
Hide resolved
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Outdated
Show resolved
Hide resolved
integration-tests/src/test/java/com/datastax/oss/driver/core/metadata/NodeStateIT.java
Show resolved
Hide resolved
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Outdated
Show resolved
Hide resolved
Introduce RequestRoutingType to distinguish LWT from regular queries and add a new configuration option to control LWT routing behavior. The new `advanced.load-balancing-policy.default-lwt-request-routing-method` option allows choosing between: - REGULAR: Default shuffling and slow replica avoidance - PRESERVE_REPLICA_ORDER: Maintains replica order from partitioner Changes: - Add RequestRoutingType enum (REGULAR, LWT) to classify requests - Remove unused RequestRoutingMethod enum from Request interface - Thread RequestRoutingType through Statement builders and implementations - Update DefaultLoadBalancingPolicy to route LWT queries according to config - Add corresponding TypedDriverOption and OptionsMap support - Update prepared statement creation to detect and mark LWT queries - Remove RequestRoutingMethod.getRoutingMethod() default method This enables optimized LWT performance by avoiding unnecessary shuffling when replica order preservation is beneficial for linearizability. refactor: update LWT request routing method to preserve replica order
...ain/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
Outdated
Show resolved
Hide resolved
40f7257 to
d227fc4
Compare
- Fix Javadoc positioning: Move @nonnull annotations after doc comments in DefaultLoadBalancingPolicy methods (per Java conventions) - Add missing @nonnull annotation to StatementBuilderTest mock builder - Add @nullable annotation to NodeStateIT query plan method signature - Standardize test infrastructure: * Add @RunWith(MockitoJUnitRunner.Silent.class) to 7 test classes * Update LoadBalancingPolicyTestBase to stub LWT routing config option * Convert base class from @RunWith to abstract (subclasses now declare runner) - Standardize integration test naming: ccmRule→CCM_RULE, sessionRule→SESSION_RULE - Update test mocks with RequestRoutingType.REGULAR parameter for compatibility - Improve LWT integration tests: * BatchStatementIT: Fix variable references, enhance LWT batch assertions * LWTLoadBalancingIT: Change from single-node to replica-set validation * Add LWTLoadBalancingMultiDcIT: New multi-DC LWT routing test coverage No functional changes to production code—purely code quality and test improvements. Apply suggestions from code review Co-authored-by: Dmitry Kropachev <dmitry.kropachev@gmail.com>
c3889cd to
30f3b4b
Compare
| if (request == null) { | ||
| return RequestRoutingMethod.REGULAR; | ||
| } | ||
| switch (request.getRequestRoutingType()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch does not handle null, it will throw NullPointerException
| private final Node node; | ||
| private final int nowInSeconds; | ||
| private final Boolean isLWT; | ||
| private final Boolean isExplicitLWTSet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please store RoutingType instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New impl: keeping @NonNull and @Nullable for requestRoutingType, additionally moving requestRoutingType field to concrete builder classes implementing StatementBuilder abstract class since it can't fulfill both requrements.
30f3b4b to
1baa9e8
Compare
1baa9e8 to
a481bc2
Compare
| @NonNull private ImmutableList.Builder<BatchableStatement<?>> statementsBuilder; | ||
| private int statementsCount; | ||
| @Nullable private Boolean isLWT = null; | ||
| @Nullable protected RequestRoutingType requestRoutingType = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note @Nullable with default null value.
| @NonNull private final ByteBuffer[] values; | ||
| @NonNull private final CodecRegistry codecRegistry; | ||
| @NonNull private final ProtocolVersion protocolVersion; | ||
| @NonNull protected RequestRoutingType requestRoutingType = RequestRoutingType.REGULAR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note @NonNull with default RequestRoutingType.REGULAR value.
| @Nullable protected Duration timeout; | ||
| @Nullable protected Node node; | ||
| protected int nowInSeconds = Statement.NO_NOW_IN_SECONDS; | ||
| @NonNull protected RequestRoutingType requestRoutingType = RequestRoutingType.REGULAR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing from base class since @NonNull annotation can not serve every concrete implementation (nullable for batch statements)
Addresses: #384
Summary
LWT statements previously bypassed the configured
LoadBalancingPolicyand used raw replicas from the token map, which ignored DC/rack-aware policies. This PR routes LWT through LBP, introduces explicit routing hints, and adjusts replica handling to prefer local-DC replicas and avoid rack-local bias.Motivation
In clusters using
TokenAwarewith DC/rack-aware policies, LWT traffic was coordinated against replicas in a fixed order, sometimes in a different DC or rack than intended. This led to suboptimal coordination and unexpected cross-DC behavior.Changes
RequestRoutingTypeenum:REGULAR,LWT.RequestRoutingMethodenum:REGULAR,PRESERVE_REPLICA_ORDER,TOKEN_BASED_REPLICA_SHUFFLING(hint; onlyPRESERVE_REPLICA_ORDERis honored in this PR).Request:@Nullable RequestRoutingType getRequestType()default RequestRoutingMethod getRoutingMethod() { return RequestRoutingMethod.REGULAR; }SimpleStatement,BoundStatement,BatchStatement, graph statements, andPrepareRequestnow implementgetRequestType()(LWTwhen applicable; otherwiseREGULAR).BatchStatementBuilderandDefaultBatchStatement: replaceisLWTplumbing withRequestRoutingType.isLWT()now derives from routing type or constituent statements.CqlRequestHandler; routing for both LWT and non-LWT is delegated to LBP viaDefaultLoadBalancingPolicy.newQueryPlan(...).DefaultLoadBalancingPolicy):RequestRoutingMethod.PRESERVE_REPLICA_ORDERby skipping shuffling.avoidSlowReplicasis enabled.@Nullable Requestand@Nullable Sessionto align with updated call sites.NodeStateITto accept nullable params innewQueryPlan.KeyRequest) to implementgetRequestType().Tests
LWTLoadBalancingIT: assert coordinators are a subset of replicas (not a single fixed node).LWTLoadBalancingMultiDcIT: new test verifying LWT routing prefers local-DC replicas in multi-DC setups.BatchStatementIT: strengthen LWT batch coverage; validate serial consistency,RequestRoutingType, presence of routing key, and rerun behavior.Behavior
getRoutingMethod() == PRESERVE_REPLICA_ORDER.Backward Compatibility
Request.Notes for Reviewers
RequestRoutingMethod.TOKEN_BASED_REPLICA_SHUFFLINGis introduced as a hint but not actively applied inDefaultLoadBalancingPolicyin this PR; scope limited toPRESERVE_REPLICA_ORDER.PrepareRequestreportREGULARrouting type by default.