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

Java: Add PING command #914

Merged
merged 14 commits into from
Feb 9, 2024
48 changes: 27 additions & 21 deletions java/client/src/main/java/glide/api/BaseClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,34 +93,40 @@ protected static CommandManager buildCommandManager(ChannelHandler channelHandle
}

/**
* Extracts the response from the Protobuf response and either throws an exception or returns the
* appropriate response as an <code>Object</code>.
* Extracts the value from a Redis response message and either throws an exception or returns the
* value as an object of type {@link T}. If <code>isNullable</code>, than also returns <code>null
* </code>.
*
* @param response Redis protobuf message
* @return Response <code>Object</code>
* @param classType Parameter {@link T} class type
* @param isNullable Accepts null values in the protobuf message
* @return Response as an object of type {@link T} or <code>null</code>
* @param <T> return type
* @throws RedisException on a type mismatch
*/
private <T> T handleRedisResponse(Class classType, boolean isNullable, Response response) {
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
private <T> T handleRedisResponse(Class classType, boolean isNullable, Response response) {
protected <T> T handleRedisResponse(Class<T> classType, boolean isNullable, Response response) throws RedisException {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @shachlanAmazon for the idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 2cbf558

Object value =
new BaseCommandResponseResolver(RedisValueResolver::valueFromPointer).apply(response);
if (isNullable && (value == null)) {
return null;
}
if (classType.isInstance(value)) {
return (T) value;
}
String className = value == null ? "null" : value.getClass().getSimpleName();
throw new RedisException(
"Unexpected return type from Redis: got "
+ className
+ " expected "
+ classType.toGenericString());
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved
}

protected Object handleObjectResponse(Response response) {
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
// convert protobuf response into Object
return new BaseCommandResponseResolver(RedisValueResolver::valueFromPointer).apply(response);
return handleRedisResponse(Object.class, false, response);
}

/**
* Extracts the response value from the Redis response and either throws an exception or returns
* the value as a <code>String</code>.
*
* @param response Redis protobuf message
* @return Response as a <code>String</code>
* @throws RedisException if there's a type mismatch
*/
protected String handleStringResponse(Response response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not necessarily for this PR - this has a very similar structure to the handleArrayResponse.
Consider making a generic function with some refinement of this imaginary code:

protected T handleResponse<T>(Response response) {
        Object value = handleObjectResponse(response);
        if (value instanceof T || value == null) {
            return (T) value;
        }
        throw new RedisException(
                "Unexpected return type from Redis: got "
                        + value.getClass().getSimpleName()
                        + " expected " + T.getType());
    }
    ```
of course, `T.getType()` isn't really possible and this doesn't handle primitives well, but I believe this is possible, and will remove a lot of glue code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea. I've updated to include the generic function - this will reduce code duplication.

Object value = handleObjectResponse(response);
if (value instanceof String || value == null) {
return (String) value;
}
throw new RedisException(
"Unexpected return type from Redis: got "
+ value.getClass().getSimpleName()
+ " expected String");
return handleRedisResponse(String.class, false, response);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should make sections/regions - for handlers, for commands for all rest

Copy link
Contributor Author

@acarbonetto acarbonetto Feb 7, 2024

Choose a reason for hiding this comment

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

Sure - but let's leave this for a future update. This will be useful when we have more than 3 commands.

@Override
Expand Down
Loading