Java: async `getPubSubMessage`. #1770

Yury-Fridlyand · 2024-07-02T22:26:29Z

Issue #, if available:
N/A

Description of changes:
#1662 (comment)

TODO list:

Impl
UT
IT

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

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

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

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

This reverts commit e76d655. Signed-off-by: Yury-Fridlyand <[email protected]>

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

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

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

.github/workflows/java.yml

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

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

Yury-Fridlyand · 2024-07-03T19:56:04Z

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

+            }
+        }
+
+        // redis can reorder the messages, so we can't validate that the order (without big delays


Tested with Wireshark - Not my fault ©

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

java/client/src/test/java/glide/connectors/handlers/PubSubMessageQueueTests.java

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

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

jduo

LGTM for the impl. Will examine the tests more.

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

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

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

jduo · 2024-07-05T00:52:21Z

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

+
+    /** Store a new message. */
+    public synchronized void push(PubSubMessage message) {
+        if (headPromiseRequested.getAndSet(false)) {


There's no reason to use an atomic if the only place it's used is a synchronized method. The use case for atomics is usually that you check-and-set it, then potentially lock.

I know. I keep trying to get rid of synchronized.

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

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

jduo · 2024-07-05T01:08:37Z

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

+import java.util.concurrent.atomic.AtomicBoolean;
+
+/** A FIFO message queue for {@link PubSubMessage}. */
+public class PubSubMessageQueue {


Could be package-private.

Compilation fails - BaseClient from glide.api uses it.

That seems strange that it uses it directly. I would've thought it goes through MessageHandler.

OK I see, you are creating a MessageHandler before the builder, then passing the handler in when creating the channel but passing the queue in to the client's constructor.

This is exposing a class from a non-public package to a non-private field in a public/protected class in an exposed package.

I am actually seeing that on a bunch of BaseClient classes. Let's discuss offline.

Separate from the above issue of internal classes in public APIs, I think BaseClient should not have a MessageQueue. It should always route through a MessageHandler that accesses a MessageQueue. I would MessageQueue a static inner class of MessageHandler (package-private for unit testing).

This would make it clear a MessageQueue can't really be on its own and is always tied to a MessageHandler.

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

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

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

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

ikolomi

@Yury-Fridlyand
Please make sure there are no commits such as:
"
I HATE YOU SPOTLESS
Revert some stuff.
O, great god of test, accept this.
"
They might be viewed a sign of unprofessional work by our customers and we dont want that. Better yes, squash the all the commits into one, so the whole work can be easily compartmentalized

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

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

jduo · 2024-07-05T15:07:00Z

@Yury-Fridlyand , let's rebase this on main now since it has the comment and API changes. Of particular note:

GlideStrings are now used instead of Strings for PubSubMessages.
Order of channel and message parameters swapped for publish() functions.
New integration tests in PubSubTests

Yury-Fridlyand · 2024-07-05T17:15:26Z

Commits are squashed at the moments of merge.

ikolomi

Approving make sure the commit messages are cleaned up

Yury-Fridlyand added 2 commits July 2, 2024 15:21

Add backend for async getPubSubMessage.

a6f2b29

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

Revert some stuff.

148eaf8

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

Yury-Fridlyand added the java issues and fixes related to the java client label Jul 2, 2024

Yury-Fridlyand requested a review from ikolomi July 2, 2024 22:26

Yury-Fridlyand mentioned this pull request Jul 2, 2024

Java: Pub & Sub = <3 #1662

Merged

5 tasks

jduo reviewed Jul 2, 2024

View reviewed changes

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

jduo reviewed Jul 2, 2024

View reviewed changes

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

jduo reviewed Jul 2, 2024

View reviewed changes

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

Yury-Fridlyand added 6 commits July 2, 2024 23:22

Address PR comments.

512eaa7

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

Test fixes.

1586794

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

Revert a test stub.

9b0cef3

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

Activate tests on mac.

e76d655

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

I HATE YOU SPOTLESS

e0ca0a4

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

Revert "Activate tests on mac."

025de96

This reverts commit e76d655. Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand commented Jul 3, 2024

View reviewed changes

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

java/client/src/main/java/glide/api/BaseClient.java Outdated Show resolved Hide resolved

Yury-Fridlyand added 2 commits July 3, 2024 12:50

O, great god of test, accept this.

eb84c2a

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

Code cleanup.

b11e444

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

Yury-Fridlyand commented Jul 3, 2024

View reviewed changes

Yury-Fridlyand marked this pull request as ready for review July 3, 2024 19:56

Yury-Fridlyand requested a review from a team as a code owner July 3, 2024 19:56

Typo fix.

8ee4741

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

Yury-Fridlyand commented Jul 3, 2024

View reviewed changes

java/client/src/test/java/glide/connectors/handlers/PubSubMessageQueueTests.java Show resolved Hide resolved

jduo reviewed Jul 3, 2024

View reviewed changes

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

jduo reviewed Jul 3, 2024

View reviewed changes

java/integTest/src/test/java/glide/PubSubTests.java Show resolved Hide resolved

jduo approved these changes Jul 3, 2024

View reviewed changes

Yury-Fridlyand added 2 commits July 3, 2024 16:07

Address PR comments.

87e5739

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

Polish the tests.

72da45a

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

ikolomi requested changes Jul 4, 2024

View reviewed changes

java/client/src/main/java/glide/api/BaseClient.java Outdated Show resolved Hide resolved

Yury-Fridlyand added 2 commits July 4, 2024 17:09

Rework impl.

5729ca6

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

Merge branch 'main' into java/integ_yuryf_async_get_msg

a483c1f

Fix tests.

42d7907

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

jduo reviewed Jul 5, 2024

View reviewed changes

java/client/src/main/java/glide/api/BaseClient.java Outdated Show resolved Hide resolved

jduo reviewed Jul 5, 2024

View reviewed changes

java/client/src/main/java/glide/api/BaseClient.java Outdated Show resolved Hide resolved

jduo reviewed Jul 5, 2024

View reviewed changes

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

jduo reviewed Jul 5, 2024

View reviewed changes

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

jduo reviewed Jul 5, 2024

View reviewed changes

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

jduo reviewed Jul 5, 2024

View reviewed changes

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

jduo reviewed Jul 5, 2024

View reviewed changes

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

Yury-Fridlyand added 2 commits July 4, 2024 20:13

Address PR comments.

a20febd

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

Address PR comments.

899adce

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

jduo reviewed Jul 5, 2024

View reviewed changes

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

Typo fix.

5fa4368

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

jduo approved these changes Jul 5, 2024

View reviewed changes

ikolomi requested changes Jul 5, 2024

View reviewed changes

java/client/src/main/java/glide/api/BaseClient.java Outdated Show resolved Hide resolved

jduo reviewed Jul 5, 2024

View reviewed changes

java/integTest/src/test/java/glide/PubSubTests.java Show resolved Hide resolved

ikolomi approved these changes Jul 5, 2024

View reviewed changes

Yury-Fridlyand closed this Jul 5, 2024

Yury-Fridlyand deleted the java/integ_yuryf_async_get_msg branch July 5, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: async `getPubSubMessage`. #1770

Java: async `getPubSubMessage`. #1770

Yury-Fridlyand commented Jul 2, 2024 •

edited

Loading

Yury-Fridlyand Jul 3, 2024

jduo left a comment

jduo Jul 5, 2024

Yury-Fridlyand Jul 5, 2024

jduo Jul 5, 2024

Yury-Fridlyand Jul 5, 2024

jduo Jul 5, 2024

jduo Jul 5, 2024

jduo Jul 5, 2024

ikolomi left a comment

jduo commented Jul 5, 2024

Yury-Fridlyand commented Jul 5, 2024

ikolomi left a comment

Java: async getPubSubMessage. #1770

Java: async getPubSubMessage. #1770

Conversation

Yury-Fridlyand commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

jduo left a 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

ikolomi left a comment

Choose a reason for hiding this comment

jduo commented Jul 5, 2024

Yury-Fridlyand commented Jul 5, 2024

ikolomi left a comment

Choose a reason for hiding this comment

Java: async `getPubSubMessage`. #1770

Java: async `getPubSubMessage`. #1770

Yury-Fridlyand commented Jul 2, 2024 •

edited

Loading