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 CONFIG REWRITE and CONFIG RESETSTAT. #95

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion java/client/src/main/java/glide/api/BaseClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import static glide.ffi.resolvers.SocketListenerResolver.getSocket;
import static glide.utils.ArrayTransformUtils.castArray;
import static glide.utils.ArrayTransformUtils.convertMapToArgArray;
import static redis_request.RedisRequestOuterClass.RequestType.ConfigResetStat;
import static redis_request.RedisRequestOuterClass.RequestType.ConfigRewrite;
import static redis_request.RedisRequestOuterClass.RequestType.Decr;
import static redis_request.RedisRequestOuterClass.RequestType.DecrBy;
import static redis_request.RedisRequestOuterClass.RequestType.GetString;
Expand All @@ -24,6 +26,7 @@

import glide.api.commands.ConnectionManagementCommands;
import glide.api.commands.HashCommands;
import glide.api.commands.ServerManagementBaseCommands;
import glide.api.commands.SetCommands;
import glide.api.commands.StringCommands;
import glide.api.models.commands.SetOptions;
Expand Down Expand Up @@ -56,7 +59,8 @@ public abstract class BaseClient
ConnectionManagementCommands,
StringCommands,
HashCommands,
SetCommands {
SetCommands,
ServerManagementBaseCommands {
/** Redis simple string response with "OK" */
public static final String OK = ConstantResponse.OK.toString();

Expand Down Expand Up @@ -309,4 +313,16 @@ public CompletableFuture<Set<String>> smembers(String key) {
public CompletableFuture<Long> scard(String key) {
return commandManager.submitNewCommand(SCard, new String[] {key}, this::handleLongResponse);
}

@Override
public CompletableFuture<String> configRewrite() {
return commandManager.submitNewCommand(
ConfigRewrite, new String[0], this::handleStringResponse);
}

@Override
public CompletableFuture<String> configResetStat() {
return commandManager.submitNewCommand(
ConfigResetStat, new String[0], this::handleStringResponse);
}
}
24 changes: 24 additions & 0 deletions java/client/src/main/java/glide/api/RedisClusterClient.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */
package glide.api;

import static redis_request.RedisRequestOuterClass.RequestType.ConfigResetStat;
import static redis_request.RedisRequestOuterClass.RequestType.ConfigRewrite;
import static redis_request.RedisRequestOuterClass.RequestType.CustomCommand;
import static redis_request.RedisRequestOuterClass.RequestType.Info;
import static redis_request.RedisRequestOuterClass.RequestType.Ping;
Expand Down Expand Up @@ -132,4 +134,26 @@ public CompletableFuture<ClusterValue<String>> info(
? ClusterValue.of(handleStringResponse(response))
: ClusterValue.of(handleMapResponse(response)));
}

@Override
public CompletableFuture<String> configRewrite() {

Choose a reason for hiding this comment

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

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...

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

reverted

return super.configRewrite();
}

@Override
public CompletableFuture<String> configRewrite(@NonNull Route route) {
return commandManager.submitNewCommand(
ConfigRewrite, new String[0], route, this::handleStringResponse);
}

@Override
public CompletableFuture<String> configResetStat() {
return super.configResetStat();
}

@Override
public CompletableFuture<String> configResetStat(@NonNull Route route) {
return commandManager.submitNewCommand(
ConfigResetStat, new String[0], route, this::handleStringResponse);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */
package glide.api.commands;

import java.util.concurrent.CompletableFuture;

/**
* Server Management Commands interface for both standalone and cluster clients.
*
* @see <a href="https://redis.io/commands/?group=server">Server Management Commands</a>
*/
public interface ServerManagementBaseCommands {

/**
* Rewrites the configuration file with the current configuration.
*
* @see <a href="https://redis.io/commands/config-rewrite/">redis.io</a> for details.
* @return <code>OK</code> when the configuration was rewritten properly, otherwise an error is
* raised.
* @example
* <pre>
* String response = client.configRewrite().get();
* assert response.equals("OK")
* </pre>
*/
CompletableFuture<String> configRewrite();

/**
* Resets the statistics reported by Redis using the <a
* href="https://redis.io/commands/info/">INFO</a> and <a
* href="https://redis.io/commands/latency-histogram/">LATENCY HISTOGRAM</a> commands.
*
* @see <a href="https://redis.io/commands/config-resetstat/">redis.io</a> for details.
* @return <code>OK</code> to confirm that the statistics were successfully reset.
* @example
* <pre>
* String response = client.configResetStat().get();
* assert response.equals("OK")
* </pre>
*/
CompletableFuture<String> configResetStat();
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import java.util.concurrent.CompletableFuture;

/**
* Server Management Commands interface.
* Server Management Commands interface for cluster client.
*
* @see <a href="https://redis.io/commands/?group=server">Server Management Commands</a>
*/
Expand Down Expand Up @@ -71,4 +71,68 @@ public interface ServerManagementClusterCommands {
* value is the information of the sections requested for the node.
*/
CompletableFuture<ClusterValue<String>> info(InfoOptions options, Route route);

/**
* Rewrites the configuration file with the current configuration.<br>
* The command will be routed automatically to all nodes.
*
* @see <a href="https://redis.io/commands/config-rewrite/">redis.io</a> for details.
* @return <code>OK</code> when the configuration was rewritten properly, otherwise an error is
* raised.
* @example
* <pre>
* String response = client.configRewrite().get();
* assert response.equals("OK")
* </pre>
*/
CompletableFuture<String> configRewrite();

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

reverted


/**
* Rewrites the configuration file with the current configuration.
*
* @see <a href="https://redis.io/commands/config-rewrite/">redis.io</a> for details.
* @param route Routing configuration for the command. Client will route the command to the nodes
* defined.
* @return <code>OK</code> when the configuration was rewritten properly, otherwise an error is
* raised.
* @example
* <pre>
* String response = client.configRewrite(ALL_PRIMARIES).get();
* assert response.equals("OK")
* </pre>
*/
CompletableFuture<String> configRewrite(Route route);

/**
* Resets the statistics reported by Redis using the <a
* href="https://redis.io/commands/info/">INFO</a> and <a
* href="https://redis.io/commands/latency-histogram/">LATENCY HISTOGRAM </a> commands.<br>
* The command will be routed automatically to all nodes.
*
* @see <a href="https://redis.io/commands/config-resetstat/">redis.io</a> for details.
* @return <code>OK</code> to confirm that the statistics were successfully reset.
* @example
* <pre>
* String response = client.configResetStat().get();
* assert response.equals("OK")
* </pre>
*/
CompletableFuture<String> configResetStat();

/**
* Resets the statistics reported by Redis using the <a
* href="https://redis.io/commands/info/">INFO</a> and <a
* href="https://redis.io/commands/latency-histogram/">LATENCY HISTOGRAM </a> commands.
*
* @see <a href="https://redis.io/commands/config-resetstat/">redis.io</a> for details.
* @param route Routing configuration for the command. Client will route the command to the nodes
* defined.
* @return <code>OK</code> to confirm that the statistics were successfully reset.
* @example
* <pre>
* String response = client.configResetStat(ALL_PRIMARIES).get();
* assert response.equals("OK")
* </pre>
*/
CompletableFuture<String> configResetStat(Route route);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import java.util.concurrent.CompletableFuture;

/**
* Server Management Commands interface.
* Server Management Commands interface for standalone client.
*
* @see <a href="https://redis.io/commands/?group=server">Server Management Commands</a>
*/
Expand Down
27 changes: 27 additions & 0 deletions java/client/src/main/java/glide/api/models/BaseTransaction.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
package glide.api.models;

import static glide.utils.ArrayTransformUtils.convertMapToArgArray;
import static redis_request.RedisRequestOuterClass.RequestType.ConfigResetStat;
import static redis_request.RedisRequestOuterClass.RequestType.ConfigRewrite;
import static redis_request.RedisRequestOuterClass.RequestType.CustomCommand;
import static redis_request.RedisRequestOuterClass.RequestType.Decr;
import static redis_request.RedisRequestOuterClass.RequestType.DecrBy;
Expand Down Expand Up @@ -413,6 +415,31 @@ public T scard(String key) {
return getThis();
}

/**
* Rewrite the configuration file with the current configuration.
*
* @see <a href="https://redis.io/commands/config-rewrite/">redis.io</a> for details.
* @return <code>OK</code> when the configuration was rewritten properly, otherwise an error is
* raised.
*/
public T configRewrite() {
protobufTransaction.addCommands(buildCommand(ConfigRewrite));
return getThis();
}

/**
* Reset the statistics reported by Redis using the <a
* href="https://redis.io/commands/info/">INFO</a> and <a
* href="https://redis.io/commands/latency-histogram/">LATENCY HISTOGRAM </a> commands.
*
* @see <a href="https://redis.io/commands/config-resetstat/">redis.io</a> for details.
* @return <code>OK</code> to confirm that the statistics were successfully reset.
*/
public T configResetStat() {
protobufTransaction.addCommands(buildCommand(ConfigResetStat));
return getThis();
}

/** Build protobuf {@link Command} object for given command and arguments. */
protected Command buildCommand(RequestType requestType) {
return buildCommand(requestType, buildArgs());
Expand Down
43 changes: 38 additions & 5 deletions java/client/src/test/java/glide/api/RedisClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static redis_request.RedisRequestOuterClass.RequestType.ConfigResetStat;
import static redis_request.RedisRequestOuterClass.RequestType.ConfigRewrite;
import static redis_request.RedisRequestOuterClass.RequestType.CustomCommand;
import static redis_request.RedisRequestOuterClass.RequestType.Decr;
import static redis_request.RedisRequestOuterClass.RequestType.DecrBy;
Expand Down Expand Up @@ -328,11 +330,6 @@ public void mset_returns_success() {

// exercise
CompletableFuture<String> response = service.mset(keyValueMap);
String payload = response.get();

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

@SneakyThrows
Expand Down Expand Up @@ -622,4 +619,40 @@ public void scard_returns_success() {
assertEquals(testResponse, response);
assertEquals(value, payload);
}

@SneakyThrows
@Test
public void configRewrite_returns_success() {
// setup
CompletableFuture<String> testResponse = mock(CompletableFuture.class);
when(testResponse.get()).thenReturn(OK);
when(commandManager.<String>submitNewCommand(eq(ConfigRewrite), eq(new String[0]), any()))
.thenReturn(testResponse);

// exercise
CompletableFuture<String> response = service.configRewrite();
String payload = response.get();

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

@SneakyThrows
@Test
public void configResetStat_returns_success() {
// setup
CompletableFuture<String> testResponse = mock(CompletableFuture.class);
when(testResponse.get()).thenReturn(OK);
when(commandManager.<String>submitNewCommand(eq(ConfigResetStat), eq(new String[0]), any()))
.thenReturn(testResponse);

// exercise
CompletableFuture<String> response = service.configResetStat();
String payload = response.get();

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I reverted this and added new tests

}
}
Loading
Loading