Skip to content

Benchmark: Handle valkey-glide primary driver commit ID#66

Merged
liorsve merged 2 commits intomainfrom
handle-primary-driver-version
Mar 15, 2026
Merged

Benchmark: Handle valkey-glide primary driver commit ID#66
liorsve merged 2 commits intomainfrom
handle-primary-driver-version

Conversation

@jeremyprime
Copy link
Copy Markdown
Collaborator

@jeremyprime jeremyprime commented Mar 3, 2026

Extends primary driver commit ID support to valkey-glide. Jedis, Lettuce, and other drivers only support a maven version.

Tested by running benchmarks against a published version and a commit hash in the valkey-glide repo.

@jeremyprime jeremyprime requested review from ikolomi and liorsve March 3, 2026 22:37
@jeremyprime jeremyprime force-pushed the handle-primary-driver-version branch from 045ba5d to 58e4c1f Compare March 4, 2026 17:21
@jeremyprime jeremyprime changed the title Benchmark: Handle primary driver commit ID Benchmark: Handle valkey-glide primary driver commit ID Mar 4, 2026
echo "version=$SDV_VERSION" >> $GITHUB_OUTPUT

- name: Install valkey-glide build dependencies
- name: Install valkey-glide build dependencies (primary)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These two install dependencies + build glide steps in the primary case are identical to the two existing steps of installing + building in the secondary case. You can just modify the condition for the already existing steps to include the primary case, and make sure the Update resp-bench driver versions step handles replacing the versions in the right places.

Copy link
Copy Markdown
Collaborator Author

@jeremyprime jeremyprime Mar 12, 2026

Choose a reason for hiding this comment

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

I merged the dependencies steps, but cannot do the same for the build steps as the step ids (build-glide-primary and build-glide-secondary) must be different as they are used by other steps in the workflow.

# Otherwise, update the version supplied in the pom.xml.
PRIMARY_VERSION="${{ steps.inputs.outputs.primary_version }}"
PRIMARY_IS_COMMIT="${{ steps.driver-info.outputs.primary_is_commit }}"
if [ "$DRIVER_ID" != "spring-data-valkey" ] && [ -n "$PRIMARY_VERSION" ]; then
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe it would be a bit cleaner to integrate it with the previous block that updates the SDV version? something like

  if [ -n "$PRIMARY_VERSION" ]; then
    case "$DRIVER_ID" in
      spring-data-valkey)
        if [ "$PRIMARY_IS_COMMIT" = "true" ]; then
          ...
        else
          ...
        fi
        ;;
      valkey-glide)
        if [ "$PRIMARY_IS_COMMIT" = "true" ]; then
          ...
        else
          ...
        fi
        ;;
      jedis|lettuce|redisson)
        ...
        ;;
    esac
  fi

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Jeremy Parr-Pearson <jeremy.parr-pearson@improving.com>
@jeremyprime jeremyprime force-pushed the handle-primary-driver-version branch from 58e4c1f to 6dc159d Compare March 12, 2026 20:42
Signed-off-by: Jeremy Parr-Pearson <jeremy.parr-pearson@improving.com>
@liorsve liorsve merged commit dc527e4 into main Mar 15, 2026
45 of 47 checks passed
@jeremyprime jeremyprime deleted the handle-primary-driver-version branch March 16, 2026 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants