Java: Address pubsub PR comments by jduo · Pull Request #1773 · valkey-io/valkey-glide

jduo · 2024-07-03T03:55:23Z

Issue #, if available:

Description of changes:
Address some of the comments in #1662 about aligning with Python:

Remove spublish() and provide a cluster-only publish() overload that takes a boolean.
Match argument order for publish() with Python.
Add missing integration tests.
Undo splitting of integration tests.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jduo · 2024-07-03T04:33:35Z

@ikolomi FYI

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

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

jduo · 2024-07-03T14:29:44Z

FYI @eifrah-aws

jduo · 2024-07-03T15:13:21Z

@ikolomi @eifrah-aws @Yury-Fridlyand
I'll work on adding the missing integration tests in a separate PR.

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

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

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

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

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

java/client/src/main/java/glide/api/models/configuration/BaseSubscriptionConfiguration.java

java/client/src/main/java/glide/connectors/handlers/MessageHandler.java

java/client/src/main/java/glide/managers/ConnectionManager.java

java/client/src/main/java/glide/connectors/handlers/MessageHandler.java

Yury-Fridlyand

I think we need a new IT which proves that GS approach works.

java/client/src/main/java/glide/connectors/handlers/ReadHandler.java

java/integTest/src/test/java/glide/PubSubTests.java

jduo · 2024-07-04T15:03:40Z

I think we need a new IT which proves that GS approach works.

All tests use the GlideString variants of functions and PubSubMessage internally use GlideString. Do you mean to test messages that are non-UTF-8-encodable?

java/client/src/main/java/glide/connectors/handlers/MessageHandler.java

Yury-Fridlyand · 2024-07-04T18:03:49Z

java/client/src/main/java/glide/connectors/handlers/ReadHandler.java

+            cause.printStackTrace();
+
+            // Unwrap. Only works for Exceptions and not Errors.
+            throw ((MessageHandler.MessageCallbackException) cause).getCause();


where it goes to after rethrowing?

Netty catches and logs it. I think Netty will re-use the thread after.

java/client/src/main/java/glide/connectors/handlers/ReadHandler.java

java/integTest/src/test/java/glide/PubSubTests.java

…null

Also add GlideString version of OK constant (named TOK from Python)

* Use GlideString for MessageHandler responses * Have GlideString overloads for PubSub comamnds return String instead of GlideString OKs.

Add the following tests from Python: * pubsub_exact_max_size_message * pubsub_sharded_max_size_message * pubsub_exact_max_size_message_callback * pubsub_sharded_max_size_message_callback

Supply a message with the exception being logged and fix the output when printing an exception

* Senders do not need subscriptions/callbacks. * Correct validating the pattern by checking that the optional is empty instead of null. * Use 512MB messages for max-size tests * Make client cleanup match the other tests

Exceptions from the callback now escape the netty handler. This will get Netty to log it. We also explicitly log the the Glide log and System.err

* Remove spublish and provide two overloads for publish * Change publish argument order to match Python * Explicitly prevent the context being non-null and the callback being null * Parameterize three_publishing_clients_same_name_with_sharded * Address PR feedback * Add GlideString versions of pubsub commands Also add GlideString version of OK constant (named TOK from Python) * PubSubMessages should use GlideStrings instead of String * Spotless * Address PR comments * Use GlideString for MessageHandler responses * Have GlideString overloads for PubSub comamnds return String instead of GlideString OKs. * Have GlideString log byte array hash * Add pubsub MaxSize integration tests (disabled) Add the following tests from Python: * pubsub_exact_max_size_message * pubsub_sharded_max_size_message * pubsub_exact_max_size_message_callback * pubsub_sharded_max_size_message_callback * Improve exception logging. Supply a message with the exception being logged and fix the output when printing an exception * Add exception-from-callback test * Make the callback exception error handling test pass * PubSub Test fixes * Senders do not need subscriptions/callbacks. * Correct validating the pattern by checking that the optional is empty instead of null. * Use 512MB messages for max-size tests * Make client cleanup match the other tests * Add pub-sub testing with non-UTF-8 data * Simplify the non-String PubSub test * Remove stale TODOs * Change pubsub callback exception handling to pass through Netty Exceptions from the callback now escape the netty handler. This will get Netty to log it. We also explicitly log the the Glide log and System.err

jduo mentioned this pull request Jul 3, 2024

Java: Pub & Sub = <3 #1662

Merged

5 tasks

Yury-Fridlyand reviewed Jul 3, 2024

View reviewed changes

jduo force-pushed the java/dev_jduo_pubsub_pr_comments branch from aeeb2c8 to f3a0600 Compare July 3, 2024 15:12

jduo marked this pull request as ready for review July 3, 2024 15:12

jduo requested a review from a team as a code owner July 3, 2024 15:12

ikolomi requested changes Jul 3, 2024

View reviewed changes

java/client/src/main/java/glide/api/RedisClusterClient.java Show resolved Hide resolved

jduo force-pushed the java/dev_jduo_pubsub_pr_comments branch from 8472e14 to f5e9262 Compare July 3, 2024 17:29

Yury-Fridlyand reviewed Jul 3, 2024

View reviewed changes

java/client/src/main/java/glide/connectors/handlers/MessageHandler.java Show resolved Hide resolved

Yury-Fridlyand reviewed Jul 3, 2024

View reviewed changes

jduo force-pushed the java/dev_jduo_pubsub_pr_comments branch from 09b5e49 to 29828b4 Compare July 4, 2024 01:30