Skip to content

Add Valkey8 support#2102

Closed
shohamazon wants to merge 1 commit intovalkey-io:mainfrom
shohamazon:valkey-8
Closed

Add Valkey8 support#2102
shohamazon wants to merge 1 commit intovalkey-io:mainfrom
shohamazon:valkey-8

Conversation

@shohamazon
Copy link
Collaborator

@shohamazon shohamazon commented Aug 8, 2024

@shohamazon shohamazon force-pushed the valkey-8 branch 3 times, most recently from 0f57133 to 9660c60 Compare August 8, 2024 11:30
@shohamazon shohamazon force-pushed the valkey-8 branch 4 times, most recently from 30943b8 to 1713afa Compare August 8, 2024 12:40
@shohamazon shohamazon force-pushed the valkey-8 branch 17 times, most recently from b834291 to 453160a Compare August 19, 2024 09:20
@shohamazon shohamazon added python 🐍 Python wrapper node 🐢 Node.js wrapper java ☕ issues and fixes related to the java client CI/CD ⚒️ CI/CD related labels Aug 19, 2024
@shohamazon shohamazon added the docs 📖 Documentation label Aug 19, 2024
@shohamazon shohamazon force-pushed the valkey-8 branch 7 times, most recently from 7552f1d to 9e56a07 Compare August 20, 2024 10:27
Signed-off-by: Shoham Elias <[email protected]>
echo 'export PATH=/usr/local/bin:$PATH' >>~/.bash_profile

- name: Verify Valkey installation and symlinks
if: ${{ !contains(inputs.engine-version, '-rc1') }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's too specific. change to

Suggested change
if: ${{ !contains(inputs.engine-version, '-rc1') }}
if: ${{ !contains(inputs.engine-version, '-rc') }}

.toLowerCase()
.contains("can't write against a read only replica"));
if (SERVER_VERSION.isGreaterThanOrEqualTo("7.9.0")) {
assertEquals(OK, clusterClient.flushall(route).get());
Copy link
Collaborator

Choose a reason for hiding this comment

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

since valkey 8.0.8 flushall can run on replicas

Comment on lines +415 to 419
if not await check_if_server_version_lt(glide_client, "7.9.0"):
cluster_mode = parse_info_response(info_res)["server_mode"]
else:
cluster_mode = parse_info_response(info_res)["redis_mode"]
expected_cluster_mode = isinstance(glide_client, GlideClusterClient)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can remove it

Comment on lines +846 to +851
assert "WATCH inside MULTI is not allowed" in str(
e
) # TODO : add an assert on EXEC ABORT

else:
assert "Command not allowed inside a transaction" in str(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can leave with "not allowed", remove the whole string message

async def check_if_server_version_lt(client: TGlideClient, min_version: str) -> bool:
# TODO: change it to pytest fixture after we'll implement a sync client
info_str = await client.info([InfoSection.SERVER])
valkey_version = parse_info_response(info_str).get("valkey_version")
Copy link
Collaborator

Choose a reason for hiding this comment

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

valkey_version = parse_info_response(info_str).get("valkey_version")
server_version = valkey_version if valkey_version is not None else parse_info_response(info_str).get("redis_version")

if (error) {
reject(error);
} else {
resolve(stdout.split("v=")[1].split(" ")[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove duplication

return cmd_args

# Try starting valkey-server first
server_name = "valkey-server"
Copy link
Collaborator

Choose a reason for hiding this comment

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

put it in a dedicate function to check which server to use

@shohamazon shohamazon closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD ⚒️ CI/CD related docs 📖 Documentation java ☕ issues and fixes related to the java client node 🐢 Node.js wrapper python 🐍 Python wrapper

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants