Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. #95

Yury-Fridlyand · 2024-02-14T23:56:42Z

Features:

CONFIG REWRITE https://redis.io/commands/config-rewrite/
CONFIG RESETSTAT https://redis.io/commands/config-resetstat/
UT consolidation for standalone and cluster client
no IT for CONFIG REWRITE, because redis in IT runs without config file

SanHalacogluImproving · 2024-02-15T19:24:59Z

java/client/src/main/java/glide/api/RedisClusterClient.java

+
+    /** {@inheritDoc} The command will be routed automatically to all nodes. */
+    @Override
+    public CompletableFuture<String> configRewrite() {


Doesn't ClusterClient extend baseClient why do you need to call it like this? We don't have the other commands such as incr etc...

I do it to redefine (extend) the javadoc. The function signature (= API) is the same.

This is confusing. I would strongly suggest that we only extend one interface with the signature, otherwise it's confusing to developers which documentation applies here.

aaron-congo · 2024-02-15T20:22:23Z

java/client/src/test/java/glide/api/RedisClientTest.java

+
+        // verify
+        assertEquals(testResponse, response);
+        assertEquals(OK, payload);


Info and ping return strings rather than OK, but this is a minor detail and maybe not worth worrying about if we're able to remove some code duplication here like you've shown

I reverted this and added new tests

java/client/src/test/java/glide/api/models/TransactionTests.java

aaron-congo · 2024-02-15T20:32:48Z

java/integTest/src/test/java/glide/SharedCommandTests.java

+        assertAll(
+                () -> assertTrue(exception.getCause() instanceof RequestException),
+                () ->
+                        assertTrue(


Shoham asked me to remove the error message checks in my PR, although they check the error message in the other clients
valkey-io#974 (comment)

Since IT runs cluster without a config file, this command always fails in IT.
Thus, this test is the only test which I can do for that command.

Docs https://redis.io/commands/config-rewrite/ say

However if the server was started without a configuration file at all, the CONFIG REWRITE will just return an error.

Remove it.
We're running against multiple version so Redis, and we don't want to check for version-specific error messages when they update Redis.

Removed in 29cdfa4

https://github.com/aws/glide-for-redis/blob/main/python/python/tests/test_async_client.py#L831 all their tests check for error messages. This includes Node and Py maybe she is mistaken?

IMO we should keep the test. In case if CI get a newer redis version with another error message, we will fix the test.
Meanwhile I do contains check, not equals

acarbonetto · 2024-02-20T23:53:25Z

java/client/src/main/java/glide/api/RedisClusterClient.java

+
+    /** {@inheritDoc} The command will be routed automatically to all nodes. */
+    @Override
+    public CompletableFuture<String> configRewrite() {


This is confusing. I would strongly suggest that we only extend one interface with the signature, otherwise it's confusing to developers which documentation applies here.

acarbonetto · 2024-02-20T23:54:46Z

java/client/src/main/java/glide/api/commands/ServerManagementClusterCommands.java

@@ -70,4 +70,32 @@ public interface ServerManagementClusterCommands {
     *     value is the information of the sections requested for the node.
     */
    CompletableFuture<ClusterValue<String>> info(InfoOptions options, Route route);
+
+    /** {@inheritDoc} The command will be routed automatically to all nodes. */
+    CompletableFuture<String> configRewrite();


you are only saving lines of documentation by extending the base commands, which adds complexity to the code. I don't recommend doing this.

java/client/src/main/java/glide/api/models/BaseTransaction.java

java/client/src/test/java/glide/api/RedisClientTest.java

acarbonetto · 2024-02-21T00:06:06Z

java/integTest/src/test/java/glide/SharedCommandTests.java

+    @SneakyThrows
+    @ParameterizedTest
+    @MethodSource("getClients")
+    public void config_reset_stat(BaseClient client) {


I don't see any tests on the python client for configResetStat

Feel free to add new 😉

Signed-off-by: Yury-Fridlyand <[email protected]>

* Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. Signed-off-by: Yury-Fridlyand <[email protected]>

* Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. (#95) --------- Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: SanHalacogluImproving <[email protected]>

Yury-Fridlyand changed the base branch from main to java/integ_yuryf_config_rewrite_reset February 14, 2024 23:57

SanHalacogluImproving reviewed Feb 15, 2024

View reviewed changes

aaron-congo approved these changes Feb 15, 2024

View reviewed changes

acarbonetto reviewed Feb 21, 2024

View reviewed changes

Yury-Fridlyand added 2 commits February 20, 2024 17:06

Add CONFIG REWRITE and CONFIG RESETSTAT.

f2b55b5

Signed-off-by: Yury-Fridlyand <[email protected]>

Rework tests and update javadocs.

97bb467

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand force-pushed the java/dev_yuryf_config_rewrite_reset branch from 08a22eb to 97bb467 Compare February 21, 2024 01:29

tense

1204e1c

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand merged commit 288590e into java/integ_yuryf_config_rewrite_reset Feb 21, 2024
10 checks passed

Yury-Fridlyand deleted the java/dev_yuryf_config_rewrite_reset branch February 21, 2024 19:51

SanHalacogluImproving pushed a commit that referenced this pull request Feb 27, 2024

Add CONFIG REWRITE and CONFIG RESETSTAT. (#95)

9571965

* Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. Signed-off-by: Yury-Fridlyand <[email protected]>

SanHalacogluImproving pushed a commit that referenced this pull request Mar 5, 2024

Add CONFIG REWRITE and CONFIG RESETSTAT. (#95)

29a7ed7

* Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. Signed-off-by: Yury-Fridlyand <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. #95

Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. #95

Yury-Fridlyand commented Feb 14, 2024 •

edited

Loading

SanHalacogluImproving Feb 15, 2024

Yury-Fridlyand Feb 15, 2024

acarbonetto Feb 20, 2024

Yury-Fridlyand Feb 21, 2024

aaron-congo Feb 15, 2024

Yury-Fridlyand Feb 21, 2024

aaron-congo Feb 15, 2024

Yury-Fridlyand Feb 15, 2024

Yury-Fridlyand Feb 15, 2024

acarbonetto Feb 15, 2024

Yury-Fridlyand Feb 15, 2024

SanHalacogluImproving Feb 15, 2024

Yury-Fridlyand Feb 15, 2024

acarbonetto Feb 20, 2024

acarbonetto Feb 20, 2024

Yury-Fridlyand Feb 21, 2024

acarbonetto Feb 21, 2024

Yury-Fridlyand Feb 21, 2024

Add CONFIG REWRITE and CONFIG RESETSTAT. #95

Add CONFIG REWRITE and CONFIG RESETSTAT. #95

Conversation

Yury-Fridlyand commented Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. #95

Add `CONFIG REWRITE` and `CONFIG RESETSTAT`. #95

Yury-Fridlyand commented Feb 14, 2024 •

edited

Loading