Skip to content

GEODE-9607: prevent pubsub from running server out of memory#6880

Merged
dschneider-pivotal merged 2 commits into
apache:developfrom
dschneider-pivotal:batchPublishRequests
Sep 30, 2021
Merged

GEODE-9607: prevent pubsub from running server out of memory#6880
dschneider-pivotal merged 2 commits into
apache:developfrom
dschneider-pivotal:batchPublishRequests

Conversation

@dschneider-pivotal

@dschneider-pivotal dschneider-pivotal commented Sep 17, 2021

Copy link
Copy Markdown
Contributor

Moved all of the publish code from PubSubImpl to Publisher.
This new class is able to batch the publish requests instead
of always sending them one at a time to other servers.

Also the ExecutorService used for publishing is now correctly configured (thanks to Jens for figuring this out) so that it can use more than one thread for publishing.

Both the publish and subscribe executor now do a critical memory check and will throw a LowMemoryException

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?

@dschneider-pivotal dschneider-pivotal added the redis Issues related to the geode-for-redis module label Sep 17, 2021
@dschneider-pivotal dschneider-pivotal changed the title Moved all of the publish code from PubSubImpl to Publisher. GEODE-9607: prevent pubsub from running server out of memory Sep 18, 2021
This new class is able to batch the publish requests instead
of always sending them one at a time to other servers.

publish and subscribe now first do a check to see if memory is running low

- publish and subscribe now do a critical memory check
OutOfMemoryDUnitTest verifies that publish and subscribe will fail
when memory is low
try {
internalPublish(batch);
} finally {
fillBatch();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In situations with high publish throughput from one client, is it possible that these recursive calls to fillBatch() might cause a stack overflow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could only happen if the executor is one like PublisherTest has that does not handoff the execute to another thread. But the executor the Publisher uses actually hand it off to another thread (which has its own stack). It could be rewritten to have the current thread loop around and call internalPublish again. That might be better from a context switch point of view (the executor thread does not need to go back to executor's queue but instead goes straight to the ClientPublisher queue) but it does not seem as fair as the current solution. But as far as stack overflow goes the thread will never have more than two frames in fillBatch (because the second one will handoff to another thread if it calls publishBatch). We can talk more about this to clear things up if needed.

@dschneider-pivotal
dschneider-pivotal merged commit 9377d81 into apache:develop Sep 30, 2021
@dschneider-pivotal
dschneider-pivotal deleted the batchPublishRequests branch September 30, 2021 04:04
dschneider-pivotal added a commit to dschneider-pivotal/geode that referenced this pull request Sep 30, 2021
…pache#6880)"

This reverts commit 9377d81.
It caused some problems with Windows integration tests.
@dschneider-pivotal dschneider-pivotal mentioned this pull request Sep 30, 2021
6 tasks
dschneider-pivotal added a commit that referenced this pull request Sep 30, 2021
…6880)" (#6921)

This reverts commit 9377d81.
It caused some problems with Windows integration tests.
dschneider-pivotal added a commit to dschneider-pivotal/geode that referenced this pull request Oct 1, 2021
dschneider-pivotal added a commit to dschneider-pivotal/geode that referenced this pull request Oct 7, 2021
… memory (apache#6880)" (apache#6921)"

This reverts commit 45383f0.

1. publish no longer creates extra netty listener objects
2. publish now writes directly to the netty ByteBuf instead
of delaying it using a RedisResponse object.
3. a publisher thread will now loop around and publish another
batch instead of handing it off to another thread

the batch is now optimized by conflating consecutive publish requests
on the same channel. This allows them to be written to netty as
a single message instead of multiple messages

Added a new PubSubConcurrentDUnitTest
If a publish is going to be sent to multiple netty channels,
it now writes the message once to a ByteBuf and shares that
ByteBuf with each channel.
The batch of publish requests is now optimized by combining
consecutive messages to the same channel.
Pattern publish now caches which subscribers match a particular
publish channel name so that the glob matching does not need
to be done on each publish.
Some of the foreach support has been dropped from the pubsub
manages in favor of getting a Collection of subscriptions so
that the publish channel writes can be grouped efficiently.

WIP: some of the refactoring requires some new unit test coverage
dschneider-pivotal added a commit to dschneider-pivotal/geode that referenced this pull request Oct 8, 2021
… memory (apache#6880)" (apache#6921)"

This reverts commit 45383f0.

1. publish no longer creates extra netty listener objects
2. publish now writes directly to the netty ByteBuf instead
of delaying it using a RedisResponse object.
3. a publisher thread will now loop around and publish another
batch instead of handing it off to another thread

the batch is now optimized by conflating consecutive publish requests
on the same channel. This allows them to be written to netty as
a single message instead of multiple messages

Added a new PubSubConcurrentDUnitTest
If a publish is going to be sent to multiple netty channels,
it now writes the message once to a ByteBuf and shares that
ByteBuf with each channel.
The batch of publish requests is now optimized by combining
consecutive messages to the same channel.
Pattern publish now caches which subscribers match a particular
publish channel name so that the glob matching does not need
to be done on each publish.
Some of the foreach support has been dropped from the pubsub
manages in favor of getting a Collection of subscriptions so
that the publish channel writes can be grouped efficiently.
@dschneider-pivotal dschneider-pivotal mentioned this pull request Oct 8, 2021
6 tasks
dschneider-pivotal added a commit that referenced this pull request Oct 9, 2021
Revert "Revert "GEODE-9607: prevent pubsub from running server out of memory (#6880)" (#6921)"

This reverts commit 45383f0.
It also makes the following improvements to that revert:
1. publish no longer creates extra netty listener objects
2. publish now writes directly to the netty ByteBuf instead
of delaying it using a RedisResponse object.
3. a publisher thread will now loop around and publish another
batch instead of handing it off to another thread
4. the batch is now optimized by conflating consecutive publish requests
on the same channel. This allows them to be written to netty as
a single message instead of multiple messages
5. Added a new PubSubConcurrentDUnitTest
6. If a publish is going to be sent to multiple netty channels,
it now writes the message once to a ByteBuf and shares that
ByteBuf with each channel.
7. Pattern publish now caches which subscribers match a particular
publish channel name so that the glob matching does not need
to be done on each publish.
Some of the foreach support has been dropped from the pubsub
manages in favor of getting a Collection of subscriptions so
that the publish channel writes can be grouped efficiently.
8. The integration test that was running out of memory before has been scaled down since the reason for the OOM was that publish had been optimized which allowed more requests to be queued on the server and then it would run out of memory. The test was running the lettuce client in the same JVM as the server which will never happen in the real world.
9. Found a couple of places publish was synchronizing that was not needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

redis Issues related to the geode-for-redis module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants