Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
jduo committed Jul 2, 2024
1 parent ef3f3d6 commit c0af505
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 31 deletions.
19 changes: 14 additions & 5 deletions java/client/src/main/java/glide/api/RedisClusterClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import glide.api.commands.ScriptingAndFunctionsClusterCommands;
import glide.api.commands.ServerManagementClusterCommands;
import glide.api.commands.TransactionsClusterCommands;
import glide.api.logging.Logger;
import glide.api.models.ClusterTransaction;
import glide.api.models.ClusterValue;
import glide.api.models.GlideString;
Expand Down Expand Up @@ -1011,7 +1012,7 @@ private static final class NativeClusterScanCursor
private boolean isClosed = false;

// This is for internal use only.
public NativeClusterScanCursor(String cursorHandle) {
public NativeClusterScanCursor(@NonNull String cursorHandle) {
this.cursorHandle = cursorHandle;
this.isFinished = FINISHED_CURSOR_MARKER.equals(cursorHandle);
}
Expand Down Expand Up @@ -1043,10 +1044,18 @@ protected void finalize() throws Throwable {

private void internalClose() {
if (!isClosed) {
ClusterScanCursorResolver.releaseNativeCursor(cursorHandle);

// Mark the cursor as closed to avoid double-free (if close() gets called more than once).
isClosed = true;
try {
ClusterScanCursorResolver.releaseNativeCursor(cursorHandle);
} catch (Exception ex) {
Logger.log(
Logger.Level.ERROR,
"ClusterScanCursor",
() -> "Error releasing cursor " + cursorHandle + ": " + ex.getMessage());
Logger.log(Logger.Level.ERROR, "ClusterScanCursor", ex);
} finally {
// Mark the cursor as closed to avoid double-free (if close() gets called more than once).
isClosed = true;
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,19 +162,31 @@ public interface GenericClusterCommands {
* Using the same cursor object for multiple iterations will result in the same keys or unexpected
* behavior. For more information about the Cluster Scan implementation, see <a
* href="https://github.com/aws/glide-for-redis/wiki/General-Concepts#cluster-scan">Cluster
* Scan</a>. As with the SCAN command, the method can be used to iterate over the keys in the
* database, to return all keys the database have from the time the scan started till the scan
* ends. The same key can be returned in multiple scans iteration.
* Scan</a>.
*
* <p>As with the SCAN command, the method can be used to iterate over the keys in the database,
* to return all keys that were in the database from the time the scan started until the scan
* finishes (that is, {@link ClusterScanCursor#isFinished()} returns true). When the cursor is not
* needed, call {@link ClusterScanCursor#releaseCursorHandle()} to immediately free resources tied
* to the cursor. Note that this makes the cursor unusable in subsequent calls to scan.
*
* <p>This method guarantees that all keyslots available when the first SCAN is called will be
* scanned before the cursor is finished. Any keys added after the initial scan request is made
* are not guaranteed to be scanned.
*
* <p>The same key can be returned in multiple scans iteration.
*
* @see ClusterScanCursor for more details about how to use the cursor.
* @see <a href="https://valkey.io/commands/scan">valkey.io</a> for details.
* @param cursor The cursor object that wraps the scan state. To start a new scan, create a new
* empty ClusterScanCursor using {@link ClusterScanCursor#initalCursor()}.
* @param cursor The {@link ClusterScanCursor} object that wraps the scan state. To start a new
* scan, create a new empty ClusterScanCursor using {@link ClusterScanCursor#initalCursor()}.
* @return An <code>Array</code> of <code>Objects</code>. The first element is always the {@link
* ClusterScanCursor} for the next iteration of results. To see if there is more data on the
* given cursor, call {@link ClusterScanCursor#isFinished()}. To release resources for the
* current chunk immediately, call {@link ClusterScanCursor#releaseCursorHandle()} after using
* the cursor in a call to this method. The second element is an <code>Array</code> of Objects
* where each entry is a <code>String</code> representing a key.
* the cursor in a call to this method. The cursor cannot be used in a scan again after {@link
* ClusterScanCursor#releaseCursorHandle()} has been called. The second element is an <code>
* Array</code> of Objects where each entry is a <code>String</code> representing a key.
* @example
* <pre>{@code
* // Assume key contains a set with 200 keys
Expand All @@ -201,20 +213,32 @@ public interface GenericClusterCommands {
* Using the same cursor object for multiple iterations will result in the same keys or unexpected
* behavior. For more information about the Cluster Scan implementation, see <a
* href="https://github.com/aws/glide-for-redis/wiki/General-Concepts#cluster-scan">Cluster
* Scan</a>. As with the SCAN command, the method can be used to iterate over the keys in the
* database, to return all keys the database have from the time the scan started till the scan
* ends. The same key can be returned in multiple scans iteration.
* Scan</a>.
*
* <p>As with the SCAN command, the method can be used to iterate over the keys in the database,
* to return all keys that were in the database from the time the scan started until the scan
* finishes (that is, {@link ClusterScanCursor#isFinished()} returns true). When the cursor is not
* needed, call {@link ClusterScanCursor#releaseCursorHandle()} to immediately free resources tied
* to the cursor. Note that this makes the cursor unusable in subsequent calls to scan.
*
* <p>This method guarantees that all keyslots available when the first SCAN is called will be
* scanned before the cursor is finished. Any keys added after the initial scan request is made
* are not guaranteed to be scanned.
*
* <p>The same key can be returned in multiple scans iteration.
*
* @see ClusterScanCursor for more details about how to use the cursor.
* @see <a href="https://valkey.io/commands/scan">valkey.io</a> for details.
* @param cursor The cursor object that wraps the scan state. To start a new scan, create a new
* empty ClusterScanCursor using {@link ClusterScanCursor#initalCursor()}.
* @param cursor The {@link ClusterScanCursor} object that wraps the scan state. To start a new
* scan, create a new empty ClusterScanCursor using {@link ClusterScanCursor#initalCursor()}.
* @param options The {@link ScanOptions}.
* @return An <code>Array</code> of <code>Objects</code>. The first element is always the {@link
* ClusterScanCursor} for the next iteration of results. To see if there is more data on the
* given cursor, call {@link ClusterScanCursor#isFinished()}. To release resources for the
* current chunk immediately, call {@link ClusterScanCursor#releaseCursorHandle()} after using
* the cursor in a call to this method. The second element is an <code>Array</code> of Objects
* where each entry is a <code>String</code> representing a key.
* the cursor in a call to this method. The cursor cannot be used in a scan again after {@link
* ClusterScanCursor#releaseCursorHandle()} has been called. The second element is an <code>
* Array</code> of Objects where each entry is a <code>String</code> representing a key.
* @example
* <pre>{@code
* // Assume key contains a set with 200 keys
Expand Down
28 changes: 28 additions & 0 deletions java/client/src/main/java/glide/api/logging/Logger.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
import static glide.ffi.resolvers.LoggerResolver.initInternal;
import static glide.ffi.resolvers.LoggerResolver.logInternal;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.PrintStream;
import java.util.function.Supplier;
import lombok.Getter;
import lombok.NonNull;
Expand Down Expand Up @@ -170,6 +173,31 @@ public static void log(
logInternal(level.getLevel(), logIdentifier, message);
}

/**
* Logs the provided exception or error if the provided log level is lower than the logger level.
*
* @param level The log level of the provided message.
* @param logIdentifier The log identifier should give the log a context.
* @param throwable The exception or error to log.
*/
public static void log(
@NonNull Level level, @NonNull String logIdentifier, @NonNull Throwable throwable) {
// TODO: Add the corresponding API to Python and Node.js.
log(
level,
logIdentifier,
() -> {
try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
PrintStream printStream = new PrintStream(outputStream)) {
throwable.printStackTrace(printStream);
return printStream.toString();
} catch (IOException e) {
// This can't happen with a ByteArrayOutputStream.
return null;
}
});
}

/**
* Creates a new logger instance and configure it with the provided log level and file name.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@
* resources. These resources can be released by calling {@link #releaseCursorHandle()}. However
* doing so will disallow the cursor from being used in {@link GenericClusterCommands#scan} to
* get more data.
* <p>To do this safely, follow this procedure: 1. Call {@link GenericClusterCommands#scan} with
* the cursor. 2. Call {@link #releaseCursorHandle()} with the cursor. 3. Reassign the cursor to
* the cursor returned by {@link GenericClusterCommands#scan}.
* <p>To do this safely, follow this procedure:
* <ol>
* <li>Call {@link GenericClusterCommands#scan} with the cursor.
* <li>Call {@link #releaseCursorHandle()} with the cursor.
* <li>Reassign the cursor to the cursor returned by {@link GenericClusterCommands#scan}.
* </ol>
* <pre>{@code
* ClusterScanCursor cursor = ClusterScanCursor.initialCursor();
* Object[] result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ public enum ObjectType {
STREAM("Stream");

/**
* Returns the name of the enum when communicating with the native layer.
*
* @return the name of the enum when communicating with the native layer.
*/
public String getNativeName() {
Expand Down Expand Up @@ -68,26 +66,20 @@ public boolean equals(Object o) {
}

/**
* Returns the pattern used for the <code>MATCH</code> filter.
*
* @return the pattern used for the <code>MATCH</code> filter.
*/
public String getMatchPattern() {
return matchPattern;
}

/**
* Returns the count used for the <code>COUNT</code> field. .
*
* @return the count used for the <code>COUNT</code> field.
*/
public Long getCount() {
return count;
}

/**
* Returns the type used for the <code>TYPE</code> filter.
*
* @return the type used for the <code>TYPE</code> filter.
*/
public ObjectType getType() {
Expand Down
5 changes: 4 additions & 1 deletion java/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,10 @@ pub extern "system" fn Java_glide_ffi_resolvers_ClusterScanCursorResolver_releas
) {
handle_panics(
move || {
fn release_native_cursor(env: &mut JNIEnv<'_>, cursor: JString) -> Result<(), FFIError> {
fn release_native_cursor(
env: &mut JNIEnv<'_>,
cursor: JString,
) -> Result<(), FFIError> {
let cursor_str: String = env.get_string(&cursor)?.into();
glide_core::cluster_scan_container::remove_scan_state_cursor(cursor_str);
Ok(())
Expand Down

0 comments on commit c0af505

Please sign in to comment.