QATOOLS-133 feature(dynamodb): implement loadbalancer strategy for Alternator (DynamoDB client for ScyllaDB)#28
Conversation
ca1f4d0 to
056ef77
Compare
056ef77 to
536bbbb
Compare
536bbbb to
8d3b6b0
Compare
| name: Test (Java ${{ matrix.java }}) | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: true | ||
| fail-fast: false | ||
| matrix: | ||
| java: ["21"] | ||
| scylla: ["2025.1", "2025.2"] | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: "temurin" | ||
| java-version: "${{ matrix.java }}" | ||
| - name: Install Deps | ||
| run: | | ||
| sudo apt-get update > /dev/null | ||
| sudo apt-get install -y maven > /dev/null | ||
| - name: Set up Docker | ||
| uses: docker/setup-docker-action@v4 | ||
| with: | ||
| set-host: true | ||
| cache: "maven" | ||
|
|
||
| - name: Run Tests | ||
| run: | | ||
| mvn clean install > /dev/null | ||
| mvn -ff -am test \ | ||
| -Djava.source="${{ matrix.java }}" \ | ||
| -Djava.target="${{ matrix.java }}" \ | ||
| mvn -B test \ | ||
| -Dproject.build.sourceEncoding=UTF-8 \ | ||
| -DskipTests=false \ | ||
| -Dscylla.version="${{ matrix.scylla }}" | ||
| -pl dynamodb,scylla -am | ||
|
|
||
| packaging: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
In general, the fix is to explicitly declare permissions for the workflow or for each job, granting only the minimal set needed. For a typical Java test/build workflow that just checks out code and runs Maven, contents: read is usually sufficient. If the called reusable workflow (./.github/workflows/build.yml) requires additional permissions, those should be defined inside that workflow; the calling workflow can still safely restrict itself to read‑only.
The best minimal, non‑breaking change here is to add a top‑level permissions: block just under the workflow name: (before on:). This block will apply to all jobs (test and packaging) because they do not define their own permissions. Based on the shown steps (actions/checkout, actions/setup-java, and mvn test plus a reusable build workflow), read access to repository contents is enough, so we set:
permissions:
contents: readNo imports or additional methods are required; this is purely a YAML configuration change within .github/workflows/test.yml.
| @@ -1,5 +1,8 @@ | ||
| name: "Test" | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| workflow_call: |
| name: Build Package (Java ${{ matrix.java }}) | ||
| needs: [test] | ||
| strategy: | ||
| fail-fast: true | ||
| matrix: | ||
| java: ["21"] | ||
| uses: "./.github/workflows/build.yml" | ||
| with: | ||
| version: "0.0.0-test" | ||
| java: "${{ matrix.java }}" |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
In general, the fix is to define a permissions block that explicitly restricts the GITHUB_TOKEN to the minimum required scopes. For this workflow, neither job appears to need write access to the repository or other resources; they just check out code, set up Java, and run Maven tasks. That means we can safely grant read-only access to repository contents at the workflow (root) level so it applies to both test and packaging jobs and documents the intent.
The best single fix without changing existing functionality is to add a top-level permissions: block after the on: section (before env: or jobs:) specifying contents: read. This ensures both jobs (including the reusable-workflow-based packaging job) run with a read-only token for repository contents, which is sufficient for actions/checkout and typical build/test operations. No other scopes appear necessary from the provided snippet. No imports or additional methods are required, only the YAML change in .github/workflows/test.yml.
Concretely:
- Edit
.github/workflows/test.yml. - Insert:
permissions:
contents: readbetween the on: block (line 3–11) and the env: block (line 13–14). This will apply to both test and packaging jobs, resolving the CodeQL warning about missing permissions while preserving existing behavior.
| @@ -10,6 +10,9 @@ | ||
| branches: | ||
| - master | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| env: | ||
| MAVEN_OPTS: "-Xmx2g -XX:+UseG1GC" | ||
|
|
soyacz
left a comment
There was a problem hiding this comment.
commit message is not clear. Please describe what things were done in this PR.
Address what option from docs was used https://github.com/scylladb/alternator-load-balancing/tree/master/java - because it's not clear what approach was used.
Also this doc says it should be minimal change - include in commit msg why this change is that size.
I based it loosly on that repository, its not direct copy paste. Nor its the importing the package, this is seperate implementation.
The actual code itself for Loadbalanacer is small, majority of the changes here are in tests and ensuring that loadbalancer actually works, since its tricky to prove it. |
Explain why it was not possible/feasible to use original implementation. This must be clear so in future no one tries reverting it. Not using original implementation has drawback of not testing it (what customers would do) and any fixes there won't be applied here.
Taking it to SCT could help - we should see traffic spread across all nodes (coordinator node) and taking one node down should not break the communication (this load balancer gives us HA as stated in docs) |
I tried to install with maven plugin in this repo and it constantly failed, could not compile it, some weird java error, so i ditched it and created my own implementation, this is the only reason. We can switch in the future to that maven package, and compare the results from this one, and if this one shows its better it could end up in the upstream of that package.
|
This is something we need to include in commit msg and in the code - for future reference. |
…namoDB client for ScyllaDB) Initial implementation for Loadbalancing strategy for Alternator - ScyllaDB compatible API for DynamoDB. This Code has been based on previous work done by @dkropachev and drivers team (https://github.com/scylladb/alternator-load-balancing/tree/master/java), further improved and adapted for the use in YCSB. Also it is created in a way that can be extract into separated library (if proven reliable) so that others can use it who need loadbalacing support. - [x] Since this one PR is using Java 21, `checkstyle.xml` has been updated to allow new features from Java 21 not to be counted as errors but allowed. - [x] Additional properties added needed to customize the load-balancing strategy Changes from the original Alternator Loadbalancer: - Use of Java 21 Features and Virtual Threads for managing and checking the ScyllaDB Hosts ( this can be further improved when we switch to Java 23+, as it brings a lot of improvements to the Virtual Thread API) - Fully Async implementation for both Alternator Client and Loadbalancer - Removed support for SDK v1 since YCSB migrated from it PR #24 Signed-off-by: Dusan Malusev <dusan@dusanmalusev.dev>
8d3b6b0 to
d5865b2
Compare
Initial implementation for Loadbalancing strategy for Alternator - ScyllaDB compatible API for DynamoDB.
This Code has been based on previous work done by @dkropachev and drivers team (https://github.com/scylladb/alternator-load-balancing/tree/master/java), further improved and adapted for the use in YCSB. Also it is created in a way that can be extract into separated library (if proven reliable) so that others can use it who need loadbalacing support.
checkstyle.xmlhas been updated to allow new features from Java 21 not to be counted as errors but allowed.Changes from the original Alternator Loadbalancer: