Skip to content
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

Add Valkey8 support #2102

Closed
wants to merge 1 commit into from
Closed

Add Valkey8 support #2102

wants to merge 1 commit into from

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]>
@@ -62,6 +62,7 @@ runs:
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

@@ -79,6 +79,9 @@ def get_random_string(length):
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