GEODE-9347: optimize pubSub #6814

dschneider-pivotal · 2021-08-28T05:06:22Z

Client now keeps track of channel and patterns it has subscribed to making it easier to check. This will improve performance of non-pubsub ops when a server has existing subscriptions.
if a client has any subscriptions.
The internal Subscriptions class now uses maps instead of lists. Concurrent maps are used and the synchronization in the pubsub internal implementation has been removed.
Some of the messages have been optimized by sending a canonical byte[] instead of converting a String to a byte[] for every message.
If many clients subscribe to the same pattern, publish will now only need to check that pattern once. The older code checked once per client.
Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically develop)?
Is your initial contribution a single, squashed commit?
Does gradlew build run cleanly?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

Each Client keeps track of its subscription names in a set. Each server keeps track of all its local subscriptions in a SubscriptionManager. The SubscriptionManager keeps track of Clients subscribed in a ClientSubscriptionManager.

jdeppe-pivotal

I'm still going over the changes, but wanted to ask these questions so long.

.../src/main/java/org/apache/geode/redis/internal/pubsub/AbstractClientSubscriptionManager.java

kirklund

Overall looks good, but I really want to see some unit tests for these classes:

PunsubscribeExecutor
UnsubscribeExecutor
Client
AbstractClientSubscriptionManager
AbstractSubscription
AbstractSubscriptionManager
ChannelSubscriptionManager
PatternClientSubscriptionManager
PatternSubscriptionManager

The following classes weren't changed much in this PR, but they really should already have unit
tests (they don't). This PR should at least introduce simple unit tests for what is changed in the
PR. Chores should also be planned to expand coverage on these classes, especially since they are
relatively new:

GlobPattern
PingExecutor
HScanExecutor
KeysExecutor
ScanExecutor
SScanExecutor
ExecutionHandlerContext
ChannelSubscription
PatternSubscription
PartitionedRegionFunctionExecutor

In particular, ExecutionHandlerContext is a rather large class to not have any unit tests.

...ntegrationTest/java/org/apache/geode/redis/internal/pubsub/SubscriptionsIntegrationTest.java

DonalEvans · 2021-09-02T22:33:10Z

@kirklund Regarding unit tests for the classes you mentioned; a decision was made by the folks who're working on the Geode Redis module to rely predominantly on integration tests to validate the behaviour of the Geode Redis API. Unit tests are used to test components that are not accessible via the API (such as object sizing for rebalance, the internal data structures that back the various RedisData classes etc.) but it was not considered worthwhile to also unit test methods that are covered in their entirety via integration tests.

Adding unit tests to the classes you list would represent a fundamental change in testing approach for the group working on the Geode Redis module, and as such, it feels like something that would be better brought to the dev list if it's a change that you feel really needs to be made, since it was originally a deliberate choice rather than an oversight.

dschneider-pivotal · 2021-09-03T16:56:46Z

@kirklund, thanks for the review. For this PR I'm going to focus on adding unit tests for the new classes and those that had significant changes (for example Client).

an instance of it to the new PatternClientSubscriptionManager. Now it stores an instance of Pattern obtained from a factory method on GlobPattern. To prevent this mistake from happening in the future, GlobPattern no longer permit instances to be created.

...edis/src/main/java/org/apache/geode/redis/internal/executor/pubsub/PunsubscribeExecutor.java

...redis/src/main/java/org/apache/geode/redis/internal/executor/pubsub/UnsubscribeExecutor.java

...ible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java

.../src/main/java/org/apache/geode/redis/internal/pubsub/AbstractClientSubscriptionManager.java

...le-with-redis/src/main/java/org/apache/geode/redis/internal/pubsub/AbstractSubscription.java

...s-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/pubsub/PubSubImpl.java

...ompatible-with-redis/src/main/java/org/apache/geode/redis/internal/pubsub/Subscriptions.java

...ntegrationTest/java/org/apache/geode/redis/internal/pubsub/SubscriptionsIntegrationTest.java

what subscriptions to drop when the user does not specific, down into the Subscriptions class. This allows the interactions with the client subscription lists to be centralized in Subscriptions instead of it being up in the command executor. So the executor is now simpler and the code is unit tested on Subscriptions.

the interrupt state of the thread

Instead it depends on Client

Before we had one for every Subscription and unsubscribe did not clean them up.

jdeppe-pivotal

Nice work Darrel!

kirklund

Thanks Darrel!

This is already stored in the key of the top level manager so it was not also needed in the leaf subscription object.

for patterns and one for channels. It is the manager that owns the subscription that determines if it is a pattern or channel.

dschneider-pivotal added the redis Issues related to the geode-for-redis module label Aug 28, 2021

dschneider-pivotal changed the title ~~client now maintains a subscription count~~ GEODE-9347: optimize pubSub Aug 30, 2021

reworked PubSub data structures.

d845d3b

Each Client keeps track of its subscription names in a set. Each server keeps track of all its local subscriptions in a SubscriptionManager. The SubscriptionManager keeps track of Clients subscribed in a ClientSubscriptionManager.

dschneider-pivotal force-pushed the opimizePubSub branch from 80400d9 to d845d3b Compare September 2, 2021 00:49

dschneider-pivotal marked this pull request as ready for review September 2, 2021 00:53

dschneider-pivotal requested review from DonalEvans, boglesby, jdeppe-pivotal, kirklund, nabarunnag, nonbinaryprogrammer, ringles and upthewaterspout as code owners September 2, 2021 00:53

jdeppe-pivotal reviewed Sep 2, 2021

View reviewed changes

fixed atomic size logic

a286a00

kirklund requested changes Sep 2, 2021

View reviewed changes

...ntegrationTest/java/org/apache/geode/redis/internal/pubsub/SubscriptionsIntegrationTest.java Outdated Show resolved Hide resolved

dschneider-pivotal added 2 commits September 2, 2021 16:49

improved coverage of SubscriptionsJUnitTest

e719dae

made test field private

9a0af40

dschneider-pivotal added 3 commits September 3, 2021 10:14

added Client unit test

4922ade

removed unneeded whitespace change

1a625b1

DonalEvans requested changes Sep 3, 2021

View reviewed changes

dschneider-pivotal added 3 commits September 3, 2021 16:51

fixed compile error

330c2d3

review feedback changes

c3a351e

DonalEvans approved these changes Sep 7, 2021

View reviewed changes

dschneider-pivotal added 2 commits September 7, 2021 15:59

pubSub interrupt handling now correctly resets

b4ed3a0

the interrupt state of the thread

pubsub code no longer depends on ExecutionHandlerContext.

4e5fe9d

Instead it depends on Client

dschneider-pivotal added 3 commits September 7, 2021 17:19

removed some debugging code

3f77868

Now we have a single channel shutdown listener per Client.

c777867

Before we had one for every Subscription and unsubscribe did not clean them up.

removed unused mocking

9b82496

jdeppe-pivotal approved these changes Sep 8, 2021

View reviewed changes

dschneider-pivotal added 7 commits September 8, 2021 12:19

added PubSubImpl unit test

910e2a0

removed unneeded subscriptions parameter from publishMessage

110837a

added javadocs

5d50daa

cleaned up Client shutdown listener code

26b68a2

added *Subscription unit tests

92a1c7c

started manager test

8672518

added more unit tests to ChannelSubscriptionManagerTest

254be81

kirklund approved these changes Sep 9, 2021

View reviewed changes

dschneider-pivotal added 6 commits September 9, 2021 14:02

added unit tests for SubscriptionManager classes

c1f6d82

added comment

0c96de4

Subscription no longer stores reference to Client

71ff653

Subscription no longer stores a reference to the channel or pattern.

be1c42d

This is already stored in the key of the top level manager so it was not also needed in the leaf subscription object.

Now have just one implementation of Subscription instead of one

e5dcf8d

for patterns and one for channels. It is the manager that owns the subscription that determines if it is a pattern or channel.

added unit tests for ClientSubscriptionManager classes

7376145

dschneider-pivotal merged commit 5cbb203 into apache:develop Sep 13, 2021

dschneider-pivotal deleted the opimizePubSub branch September 13, 2021 05:55

GEODE-9347: optimize pubSub #6814

GEODE-9347: optimize pubSub #6814

Uh oh!

Conversation

dschneider-pivotal commented Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For all changes:

Note:

Uh oh!

jdeppe-pivotal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kirklund left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DonalEvans commented Sep 2, 2021

Uh oh!

dschneider-pivotal commented Sep 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jdeppe-pivotal left a comment

Choose a reason for hiding this comment

Uh oh!

kirklund left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dschneider-pivotal commented Aug 28, 2021 •

edited

Loading