QATOOLS-133 feature(dynamodb): implement loadbalancer strategy for Alternator (DynamoDB client for ScyllaDB)

[CodeLieutenant]

Initial implementation for Loadbalancing strategy for Alternator - ScyllaDB compatible API for DynamoDB.

Using the official Alternator Java Load-Balancer

• Since this one PR is using Java 21 (mainly the features up to Java 21 and the new Virtual Thread Executor), checkstyle.xml has been updated to allow new features from Java 21 not to be counted as errors but allowed.
• Additional properties added needed to customize the load-balancing strategy

SCT Tests:

• 🟢 https://argus.scylladb.com/tests/scylla-cluster-tests/e7f70728-6902-4c6c-b67d-3db96a2fe94b

[soyacz]

Due my limited Java knowledge, I'm pasting AI review I did locally (first try) - please confirm it is right (and point errors). Some of these arch remarks should go to commit message (especially from modernization and arch section). Also add a bit info to the Readme.md in Dynamodb dir - so it's clear how to enable this load balancing (or inform that it is on, with link to repo with it).

AI Review Summary:

1. Modernization & Architecture (Excellent)

• Java 21 Adoption: The code heavily utilizes Java 21 features (records, switch expressions, virtual threads), making it cleaner and more readable.
• Shared Client Resource: The introduction of a reference-counted sharedClient (lines 142-167) is a critical improvement. It prevents the resource exhaustion issues (too many threads/connections) that
  commonly occurred when YCSB created a new DynamoDBClient for every worker thread.
• Async & Netty: Switching to DynamoDbAsyncClient with NettyNioAsyncHttpClient (lines 212-213) significantly improves throughput potential compared to the blocking Apache client.
• Virtual Threads: Explicit support for virtual threads (lines 235-240) is a forward-thinking addition for high-concurrency workloads.

2. Load Balancing Feature

• Integration: The integration of com.scylladb.alternator.load-balancing is implemented correctly via the DynamoDbEndpointProvider interface.
• Flexibility: It supports two modes:
  • Dynamic Discovery: Uses AlternatorEndpointProvider to automatically find nodes.
  • Static List: Uses a custom StaticRoundRobinEndpointProvider if dynamodb.alternator.staticNodes is provided (though this property is missing from the default dynamodb.properties).

3. Test Coverage

• Integration Tests: The new DynamoDBClientIntegrationTest correctly uses Testcontainers with ScyllaDB to verify functionality.
• Discrepancy: There is a confusing property in the tests: dynamodb.alternator.usePackageLoadBalancer.
  • The test sets this to false in one case and true in another.
  • However, the production code in DynamoDBClient.java does not appear to read this property. It relies solely on dynamodb.alternator.loadbalancing and dynamodb.alternator.staticNodes.
  • This implies that both test methods (loadBalancingModeWorks and packageLoadBalancingModeWorks) are likely testing the exact same code path (the dynamic AlternatorEndpointProvider), making one of them
    redundant or misleading.

4. Documentation

• dynamodb/conf/dynamodb.properties was updated, but it is missing the dynamodb.alternator.staticNodes property which is implemented in the code.

Actionable Recommendations

1. Fix Test Logic: Review DynamoDBClientIntegrationTest. If usePackageLoadBalancer was intended to switch between internal logic and the external library, that logic is missing from DynamoDBClient.java. If
   it's not needed, remove the confusing property from the test.
2. Update Properties File: Add dynamodb.alternator.staticNodes to dynamodb/conf/dynamodb.properties so users know they can provide a static list of endpoints.
3. Verify Checkstyle: The commit message mentions updating checkstyle.xml for Java 21. Ensure this doesn't negatively impact other modules if they aren't on Java 21 yet (though YCSB seems to be moving as a
   whole).

[CodeLieutenant]

Due my limited Java knowledge, I'm pasting AI review I did locally (first try) - please confirm it is right (and point errors). Some of these arch remarks should go to commit message (especially from modernization and arch section). Also add a bit info to the Readme.md in Dynamodb dir - so it's clear how to enable this load balancing (or inform that it is on, with link to repo with it).

AI Review Summary:

1. Modernization & Architecture (Excellent)

• Java 21 Adoption: The code heavily utilizes Java 21 features (records, switch expressions, virtual threads), making it cleaner and more readable.
• Shared Client Resource: The introduction of a reference-counted sharedClient (lines 142-167) is a critical improvement. It prevents the resource exhaustion issues (too many threads/connections) that
  commonly occurred when YCSB created a new DynamoDBClient for every worker thread.
• Async & Netty: Switching to DynamoDbAsyncClient with NettyNioAsyncHttpClient (lines 212-213) significantly improves throughput potential compared to the blocking Apache client.
• Virtual Threads: Explicit support for virtual threads (lines 235-240) is a forward-thinking addition for high-concurrency workloads.

2. Load Balancing Feature

• Integration: The integration of com.scylladb.alternator.load-balancing is implemented correctly via the DynamoDbEndpointProvider interface.

• Flexibility: It supports two modes:

  • Dynamic Discovery: Uses AlternatorEndpointProvider to automatically find nodes.
  • Static List: Uses a custom StaticRoundRobinEndpointProvider if dynamodb.alternator.staticNodes is provided (though this property is missing from the default dynamodb.properties).

3. Test Coverage

• Integration Tests: The new DynamoDBClientIntegrationTest correctly uses Testcontainers with ScyllaDB to verify functionality.

• Discrepancy: There is a confusing property in the tests: dynamodb.alternator.usePackageLoadBalancer.

  • The test sets this to false in one case and true in another.
  • However, the production code in DynamoDBClient.java does not appear to read this property. It relies solely on dynamodb.alternator.loadbalancing and dynamodb.alternator.staticNodes.
  • This implies that both test methods (loadBalancingModeWorks and packageLoadBalancingModeWorks) are likely testing the exact same code path (the dynamic AlternatorEndpointProvider), making one of them
    redundant or misleading.

4. Documentation

• dynamodb/conf/dynamodb.properties was updated, but it is missing the dynamodb.alternator.staticNodes property which is implemented in the code.

Actionable Recommendations

1. Fix Test Logic: Review DynamoDBClientIntegrationTest. If usePackageLoadBalancer was intended to switch between internal logic and the external library, that logic is missing from DynamoDBClient.java. If
   it's not needed, remove the confusing property from the test.
2. Update Properties File: Add dynamodb.alternator.staticNodes to dynamodb/conf/dynamodb.properties so users know they can provide a static list of endpoints.

The first two are valid ones, I'll fix them ASAP

3. Verify Checkstyle: The commit message mentions updating checkstyle.xml for Java 21. Ensure this doesn't negatively impact other modules if they aren't on Java 21 yet (though YCSB seems to be moving as a
   whole).

This one does not make any sense, since it's passing the listing check in CI, meaning everything is Compatible with Java 21

Summary:
Incredible work, this is exactly what was done, AI found basically everything for This Project and Why it was changed.

[fruch]

@CodeLieutenant

There are so many changes here 90% of them are not related to the task at hand (or it's not clear to me that they are related), and it's all in one commit.

It's hard to review the core part of the changes related to hooking in the alternator part

I would suggest @nyh or @dkropachev reviewing this

[fruch]

@CodeLieutenant

Why all the comments stripped here ? If at any point we will want to sync with upstream, it's gonna be painful

[CodeLieutenant]

Syncing it with current master is already painful and cannot be done in one go, only cherry picked commits.
Since we are maintaining only two modules, I don't even think we use ScyllaDB module from YCSB, only DynamoDB, and given that the project is basically stale, we should not limit ourselves to using e.g. old java version just because upstream is still on Java 8.

[fruch]

we are 13 commits from the last time we synced with upstream.

I still don't understand how this alternator change is related to the java version being used... I don't see any reason to pack it with any java version changes

[CodeLieutenant]

@dkropachev Can you review?

[soyacz]

I could do review only with AI, and it didn't find any significant issues.
(update docs missing and unknown effect of java 21 on other bindings which we don't care?)
LGTM

[soyacz]

@CodeLieutenant please update with your runs you did to test this

[soyacz]

actually, I used wrong model, better one found these:

Discrepancies & Potential Issues


  A. Unused Test Property usePackageLoadBalancer
  In DynamoDBClientTest.java and DynamoDBClientIntegrationTest.java, the property dynamodb.alternator.usePackageLoadBalancer is set in several test cases:


   1 props.setProperty("dynamodb.alternator.usePackageLoadBalancer", "true");
  However, this property is never read or used in DynamoDBClient.java. The client only checks dynamodb.alternator.loadbalancing.
   - Impact: Tests like packageLoadBalancingModeWorks and loadBalancingModeWorks might be testing the exact same code path (the AlternatorDynamoDbAsyncClient) regardless of this property's value, as long as loadbalancing is true. This makes some tests potentially redundant or
     misleading.


  B. Trust All Certificates for Alternator Client
  The commit message states: "Trust all certs enabled on all clients when requested, no more TLS not supported".
   - Implementation: The configureHttpClient method correctly sets up a trust-all manager for the standard AWS SDK client.
   - Missing in Alternator: For the Alternator client path, the code calls:
   1     var builder = AlternatorConfig.builder()
   2         // ...
   3         .withTlsConfig(TlsConfig.systemDefault()) // <--- Always system default
   4         // ...
      It does not appear to apply the trustAllCertificates logic to the AlternatorDynamoDbAsyncClient. If the intention was to support self-signed certs for Alternator as well, this seems to be missing.

recommendations:

1. Verify Test Intent: Review DynamoDBClientIntegrationTest.java to clarify if usePackageLoadBalancer was intended to trigger a different behavior or if it's legacy code that should be removed.
2. Check TLS Support: Confirm if AlternatorDynamoDbAsyncClient supports a custom TlsConfig or TrustManager that mimics the "trust all" behavior implemented for the standard client, and apply it if dynamodb.alternator.trustAllCertificates is true.