Java: async getPubSubMessage.#1770
Conversation
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
This reverts commit e76d655. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Show resolved
Hide resolved
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
| } | ||
| } | ||
|
|
||
| // redis can reorder the messages, so we can't validate that the order (without big delays |
There was a problem hiding this comment.
Tested with Wireshark - Not my fault ©
java/client/src/test/java/glide/connectors/handlers/PubSubMessageQueueTests.java
Show resolved
Hide resolved
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Outdated
Show resolved
Hide resolved
jduo
left a comment
There was a problem hiding this comment.
LGTM for the impl. Will examine the tests more.
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
|
|
||
| /** Store a new message. */ | ||
| public synchronized void push(PubSubMessage message) { | ||
| if (headPromiseRequested.getAndSet(false)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I know. I keep trying to get rid of synchronized.
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Outdated
Show resolved
Hide resolved
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| /** A FIFO message queue for {@link PubSubMessage}. */ | ||
| public class PubSubMessageQueue { |
There was a problem hiding this comment.
Compilation fails - BaseClient from glide.api uses it.
There was a problem hiding this comment.
That seems strange that it uses it directly. I would've thought it goes through MessageHandler.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
java/client/src/main/java/glide/connectors/handlers/PubSubMessageQueue.java
Outdated
Show resolved
Hide resolved
ikolomi
left a comment
There was a problem hiding this comment.
@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
|
@Yury-Fridlyand , let's rebase this on main now since it has the comment and API changes. Of particular note:
|
|
Commits are squashed at the moments of merge. |
ikolomi
left a comment
There was a problem hiding this comment.
Approving make sure the commit messages are cleaned up
Issue #, if available:
N/A
Description of changes:
#1662 (comment)
TODO list:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.