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.

Sign up for GitHub

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

Jump to bottom

Add SCRIPT SHOW command #2171

Merged
merged 8 commits into from
Sep 12, 2024
Merged

Add SCRIPT SHOW command #2171

merged 8 commits into from
Sep 12, 2024

Conversation

shohamazon
Copy link
Collaborator

@shohamazon shohamazon commented Aug 20, 2024 •
edited
Loading

TODO:

  • add default routing to random node
  • add to is_readonly_cmd

@shohamazon shohamazon requested a review from a team as a code owner August 20, 2024 17:03
@shohamazon shohamazon added python Python wrapper node Node.js wrapper java issues and fixes related to the java client labels Aug 20, 2024
Yury-Fridlyand
Yury-Fridlyand reviewed Aug 20, 2024
View reviewed changes
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Changelog? Transaction?

java/client/src/main/java/glide/api/commands/GenericBaseCommands.java Outdated Show resolved Hide resolved
java/client/src/main/java/glide/api/commands/GenericBaseCommands.java Outdated Show resolved Hide resolved
java/client/src/main/java/glide/api/BaseClient.java
@Override
public CompletableFuture<String> scriptShow(@NonNull String sha1) {
return commandManager.submitNewCommand(
ScriptShow, new String[] {sha1}, this::handleStringResponse);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it return null if no script found? If yes, use another handler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no it panics

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right valkey-io/valkey#617
Should be added to the doc in all clients

java/integTest/src/test/java/glide/SharedCommandTests.java
@ParameterizedTest(autoCloseArguments = false)
@MethodSource("getClients")
public void scriptShow_test(BaseClient client) {
assumeTrue(SERVER_VERSION.isGreaterThanOrEqualTo("7.9.0"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 7.9? Use 8.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its release candidate so it would be changed when they will actually release it #2168

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can put version check 8.0 and tests start run on this version once everything implemented

Copy link
Collaborator

Choose a reason for hiding this comment

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

or make it a constant we just need to update the constant when it's released.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have already added this before so it wont make any difference now 🙁

node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated
* See {@link https://valkey.io/commands/script-show} for more details.
*
* @param sha1 - The SHA1 digest of the script.
* @param decoder - (Optional) {@link Decoder} type which defines how to handle the response. If not set, the default decoder from the client config will be used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @param decoder - (Optional) {@link Decoder} type which defines how to handle the response. If not set, the default decoder from the client config will be used.
* @param decoder - (Optional) {@link Decoder} type which defines how to handle the response.
* If not set, the {@link BaseClientConfiguration.defaultDecoder|default decoder} will be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I copied it from another command

Copy link
Collaborator

Choose a reason for hiding this comment

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

We're going to update it in all commands

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ohh okay

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually we decided to use DecoderOption, see #2234

node/tests/SharedTests.ts Show resolved Hide resolved
python/python/tests/test_async_client.py Show resolved Hide resolved
@asafpamzn asafpamzn mentioned this pull request Aug 25, 2024
Closed
7 tasks
@shohamazon shohamazon added the Valkey-8 Support for Valkey-8 features label Sep 9, 2024
@shohamazon
Copy link
Collaborator Author

Changelog? Transaction?

will add changelog, we dont have scripts in transaction

@shohamazon shohamazon added the Release blocker Can't release without. label Sep 11, 2024
acarbonetto
acarbonetto approved these changes Sep 11, 2024
View reviewed changes
java/client/src/test/java/glide/api/GlideClientTest.java Outdated
@@ -1581,6 +1582,53 @@ public void invokeScript_with_ScriptOptionsGlideString_returns_success() {
assertEquals(payload, response.get());
}

/*@SneakyThrows
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you throw in a comment to explain why this was removed/commented out?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Enabling this test causes the scan_with_option tests to fail CI.  
TODO:  fix the scan_with_options tests and remove dependency on dynamic libraries

java/integTest/src/test/java/glide/SharedCommandTests.java
@ParameterizedTest(autoCloseArguments = false)
@MethodSource("getClients")
public void scriptShow_test(BaseClient client) {
assumeTrue(SERVER_VERSION.isGreaterThanOrEqualTo("7.9.0"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

or make it a constant we just need to update the constant when it's released.

node/src/BaseClient.ts
* ```
*/
public async scriptShow(
sha1: GlideString,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: is there a reason why we don't pass the Script object and internally we can call script.getHash()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as script exists, kill ect...

Copy link
Collaborator

Choose a reason for hiding this comment

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

well... the comment could apply to any of these commands.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea 🙉

acarbonetto
acarbonetto reviewed Sep 11, 2024
View reviewed changes
java/client/src/test/java/glide/api/GlideClientTest.java Outdated
public void scriptShow_returns_script_source_glidestring() {
// setup
GlideString scriptSource = gs("return { KEYS[1], ARGV[1] }");
Script script = mock(Script.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove
Your unit tests are probably failing because of this mock. And script is not needed anywhere.

shohamazon and others added 5 commits September 12, 2024 07:32
@shohamazon
Add SCRIPT SHOW command
b06908e 
Signed-off-by: Shoham Elias <[email protected]>
@shohamazon @Yury-Fridlyand
Apply suggestions from code review
d4cf84a 
Co-authored-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
@shohamazon
pr comments
5e994a7 
Signed-off-by: Shoham Elias <[email protected]>
@shohamazon
core
b667e6c 
Signed-off-by: Shoham Elias <[email protected]>
@shohamazon
remove thisssss
cb110a0 
Signed-off-by: Shoham Elias <[email protected]>
@shohamazon
run linter
d1b7c50 
Signed-off-by: Shoham Elias <[email protected]>
@shohamazon shohamazon force-pushed the script-show branch 2 times, most recently from 8929f01 to af8f6ec Compare September 12, 2024 09:20
@shohamazon
final
2737476 
Signed-off-by: Shoham Elias <[email protected]>
@shohamazon shohamazon merged commit 7d5de72 into valkey-io:main Sep 12, 2024
42 checks passed
@shohamazon shohamazon deleted the script-show branch September 12, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acarbonetto acarbonetto acarbonetto approved these changes

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand left review comments

Assignees
No one assigned
Labels
java issues and fixes related to the java client node Node.js wrapper python Python wrapper Release blocker Can't release without. Valkey-8 Support for Valkey-8 features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shohamazon @Yury-Fridlyand @acarbonetto