Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add autopipeline for commands and allow multi slot pipelines. #1201

Merged
merged 4 commits into from
Oct 23, 2020
Merged

Add autopipeline for commands and allow multi slot pipelines. #1201

merged 4 commits into from
Oct 23, 2020

Conversation

ShogunPanda
Copy link
Contributor

Hello all!

This PR contains the three modifications to ioredis.

Hope this helps!
Paolo

Custom commands in cluster pipelines

This fixes #536.

To make sure commands are present on the server, a SCRIPT LOAD for each command is always issued before sending the pipeline (in contrast with single node mode, where it is checked via EVALSHA).
In order not to impact performance, each SCRIPT LOAD for a command is sent at most once every options.maxScriptsCachingTime milliseconds (a new option which default is 60 seconds).

Allow pipelines in cluster mode using different slots

Pipelines don’t map to any specific Redis command, but they just control how data is sent to the server. This means that a pipeline can contain commands targeting keys belonging to different slots, as long as the slot is assigned to the same nodes group (master and its replicas).

This PR removes the existing limitation where all pipelines commands should target the same host.

In order to be able to verify that different slots are served by the same nodes group, if no slots information is available the pipeline is deferred until the cluster is connected.

Note that the same slot limitation within a single command still holds, as it is a Redis limitation.

Auto pipelining

The two features above are required to enable the last and most important new feature of this PR.

Currently, when you issue multiple commands without wrapping in pipelines, ioredis sends them to the server one by one. As described in Redis pipeline documentation, this is a suboptimal use of the network link, especially when such link is not very performant.

All the TCP and network overhead negatively affects performance. Commands are stuck in the send queue until the previous ones are correctly delivered to the server. This is a problem known as Head-Of-Line blocking (HOL).

The PR adds a feature called “auto pipelining”. It can be enabled by setting the new option enableAutoPipelining to true. No other code change is necessary.

When enabled, all commands issued during an event loop iteration are enqueued in a pipeline managed by ioredis. At the end of the iteration, the pipeline is executed and thus all commands are sent to the server at the same time.

While an automatic pipeline is executing, all new commands will be enqueued in a new pipeline which will be executed as soon as the previous finishes.

This feature can dramatically improve throughput and avoids HOL blocking. In our benchmarks, the improvement was between 35% and 50%.

Autopipelining - Single

When using Redis Cluster, one pipeline per node is created. Commands are assigned to pipelines according to which node group serves the slot. A pipeline will thus contain commands using different slots but that ultimately are assigned to the same node group.

Autopipelining - Cluster

Additionally, there is a new autoPipeliningIgnoredCommands option available to specify which commands should not automatically be wrapped in pipelines.

Credits

This PR is co-authored by @mcollina and it is based on his work in https://github.com/mcollina/ioredis-auto-pipeline.

Fixes #536.

Co-Authored-By: Matteo Collina <matteo.collina@gmail.com>
@azinoviev
Copy link
Contributor

Hi @ShogunPanda! I'm not a maintainer, but it looks like this PR needs some prettier :)

@ShogunPanda
Copy link
Contributor Author

@azinoviev Yeah, you were right. Fixed.

Comment on lines 445 to 451
this.once("ready", (...args) => {
for (const c of this._readyDelayedCallbacks) {
c(...args);
}

this._readyDelayedCallbacks = [];
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

any chance that this may hang forever in case cluster becomes unresponsive for one reason or another? not saying that this should be happening in production, but lets say a command is queued, then cluster degrades, and is that state for multiple hours. this is, of course, a sign of a deeper issue, but the command will still be queued and potentially executed later. Should there be a mechanism to flush / remove the queued call in case of a fatal failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a very good point, thanks for noticing.

First of all, let me clarify why I introduced this. With the new cluster auto pipeline routing, the Redis.Cluster instance needs to have slots-nodes information in order to assign commands to appropriate nodes group.
If these information are available (being up-to-date or not is not important thanks to MOVED and ASK support), the code above will never be invoked.

That fragment is only reached when using ioredis lazy connect feature (which is the default). Commands and/or pipelines can be prepared before the cluster server has sent slots information. So in that case I had to defer the execution until such info were ready. Thus, I listen for ready event.

I assume the ready even is only emitted once when the cluster is connected, isn't it? If that's the case, your concern above is correct, but it applies only when a Redis.Cluster instance is connected, not after. It's an edge case, but yet is possible.

So, in that case I should probably issue a .once("error", ...), am I right?

Once you conferm it, I'll made the appropriate change.
Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not 100% sure about having an error listener here, likely similar logic to https://github.com/luin/ioredis/blob/ad5b45587b2e378c15fa879cc72580c391c3c18d/lib/redis/event_handler.ts#L154 can be used here. There is a separate close handler, which would abort transactions depending on the settings (having an offline queue, for instance) - so maybe create something similar - a prop, which would hold a reference to promise or something that could be rejected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might work as well.

Another approach, by looking better at the code is to invoke callbacks with either success or failure internally in the chain in https://github.com/ShogunPanda/ioredis/blob/autopipelining/lib/cluster/index.ts#L195.

I think it's better as it is the only section in which those callbacks might be delayed.
What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

there was a bug with connect() when a cluster is not fully ready during the initial attempt - the promise would never resolve - more context here: #741

if you think that wont be a problem - then sure, an update can be added here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've updated it. Let me know if it works now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this looks awesome now 👍 @luin do you want to take a look and get this merged?
I'm not sure whether this would warrant a breaking change release or not though

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ShogunPanda @mcollina whats your personal opinion on changes - do they change the internals enough to potentially break existing integrations or I'm being overly cautious to be wanting to release this under 5.x.x?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with semver minor. I do not expect regression - we are happy to jump on any that might result immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirming what Matteo said in terms of support, let me recap few things.

The autopipeline is completely opt-in and if not enabled the code is skipped and ioredis behaves at it was.
There are only two exceptions:

  1. Cluster pipelines are always delayed until slot informations are ready
  2. Custom scripts are now always allowed in pipelines. In cluster mode, the scripts are loaded to the server before executing the pipeline.

Therefore, I also confirm this PR is compatible with semver minor release.

@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chaining custom commands in cluster mode
5 participants