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

Expose Redis#queue_key #1057

Closed
wants to merge 1 commit into from
Closed

Expose Redis#queue_key #1057

wants to merge 1 commit into from

Conversation

machty
Copy link

@machty machty commented Jan 16, 2022

Prior to this commit, the Redis class maintains a queue
mapping Thread.current.object_id to the current queue of
not-yet-committed commands for the particular thread
accessing the Redis client. This works for the common
case where a Redis client is shared amongst multiple
threads, but with Ruby 3's Fiber Scheduler and recent
strides in the Async gem, we'll want a way to choose a
different concurrency primitive (such as a Fiber-backed
Async task) to map queues to. Previously we exposed
def synchronize so that the concurrency scheme for
checking out the Redis client could be overridden in
subclasses, but internally the implicit queue key was
hardwired to Thread.current.object_id. This commit takes
a similar approach be exposing def cache_key so that it
too can be overriden by Redis subclasses.

Prior to this commit, the Redis class maintains a queue
mapping Thread.current.object_id to the current queue of
not-yet-committed commands for the particular thread
accessing the Redis client. This works for the common
case where a Redis client is shared amongst multiple
threads, but with Ruby 3's Fiber Scheduler and recent
strides in the Async gem, we'll want a way to choose a
different concurrency primitive (such as a Fiber-backed
Async task) to map queues to. Previously we exposed
`def synchronize` so that the concurrency scheme for
checking out the Redis client could be overridden in
subclasses, but internally the implicit queue key was
hardwired to Thread.current.object_id. This commit takes
a similar approach be exposing `def cache_key` so that it
too can be overriden by Redis subclasses.
@byroot
Copy link
Collaborator

byroot commented Jan 16, 2022

My plan was to just entirely get rid of that @queue thing, I think it's quite a misfeature. pipeline / multi should give you an object on which to queue your commands, instead of mutating the client.

It's just gonna take a while to deprecate.

@machty
Copy link
Author

machty commented Jan 16, 2022

I think that makes a lot of sense. I also felt the queue mutation was weird but wanted to pick the most conservative route iterating towards Fiber-safety.

@machty machty closed this Jan 16, 2022
@machty
Copy link
Author

machty commented Jan 17, 2022

@byroot def pipelined already yields self and there appear to be some tests that test this behavior. How can a block that yields an object (perhaps a Pipeline obj?) rather than a Redis instance be introduced in a backwards compatible manner? I wouldn't mind submitting the PR with some guidance here.

@byroot
Copy link
Collaborator

byroot commented Jan 17, 2022

How can a block that yields an object (perhaps a Pipeline obj?) rather than a Redis instance be introduced in a backwards compatible manner?

Like you said, it shouldn't break backward compat.

Short term we should yield a Pipeline object that respond to all the redis command methods and queue the methods, and at the end of the block we flush it.

The most important is to throw a deprecation warning if the original client is called from inside pipeline/multi or if the Pipeline object didn't queue anything.

@machty
Copy link
Author

machty commented Jan 17, 2022

@byroot the Redis class uses a dynamic method_missing to forward commands to @client; is there some definitive list of known redis commands we could consult to issue proper deprecation warnings?

@byroot
Copy link
Collaborator

byroot commented Jan 17, 2022

is there some definitive list of known redis commands we could consult to issue proper deprecation warnings?

No need. You can use proxy objects.

pipelined / multi decorate the client: https://github.com/machty/redis-rb/blob/1912288e896a7364ec235ec6abcf3081dc73518e/lib/redis.rb#L2844-L2854

You can simply decorate with a DeprecatedPipeline which raise deprecation warnings.

Alternatively, you can raise a warning if the Pipeline object you yielded is still empty when the block ends (which likely means the caller didn't use the yielded object).

Yet another method would be to check the block arity.

It's also not a strict or, some combinaison of the above might make sense.

@machty
Copy link
Author

machty commented Jan 18, 2022

I have unfortunately run through my budget on being able to work on this; got a little sucked into corner cases involving nesting that I couldn't quite wrap my head around.

Some thoughts for posterity:

  1. DeprecatedPipeline indeed seems like a promising approach
  2. There are current tests that assert that empty pipeline blocks are fine; that might thwart your idea of checking empty pipelines
  3. Even if we assert block.arity == 1, we'd probably still want checks that the global client wasn't interacted with

@machty machty deleted the queue-key branch January 18, 2022 13:29
@byroot
Copy link
Collaborator

byroot commented Jan 18, 2022

2. There are current tests that assert that empty pipeline blocks are fine; that might thwart your idea of checking empty pipelines

We can add a allow_empty: true parameter, not a huge deal. As long as it's only deprecations for now it's fine.

Even if we assert block.arity == 1, we'd probably still want checks that the global client wasn't interacted with

Definitely, it would just be a good way to catch obvious mistakes.

I have unfortunately run through my budget on being able to work on this

No worries. I'll find some time to do it.

casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 20, 2022
… block.

All theses make a lot of assumptions on the threading model and are barely
thread safe.

The new favored API is:

```ruby
redis.pipelined do |pipeline|
  pipeline.get("foo")
  pipeline.del("bar")
end
```

This API allow multiple threads to build pipelines concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 20, 2022
… block.

All theses make a lot of assumptions on the threading model and are barely
thread safe.

The new favored API is:

```ruby
redis.pipelined do |pipeline|
  pipeline.get("foo")
  pipeline.del("bar")
end
```

This API allow multiple threads to build pipelines concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 20, 2022
… block.

All theses make a lot of assumptions on the threading model and are barely
thread safe.

The new favored API is:

```ruby
redis.pipelined do |pipeline|
  pipeline.get("foo")
  pipeline.del("bar")
end
```

This API allow multiple threads to build pipelines concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
The new favored API is:

```ruby
redis.pipelined do |pipeline|
  pipeline.get("foo")
  pipeline.del("bar")
end
```

This API allow multiple threads to build pipelines concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
The new favored API is:

```ruby
redis.multi do |transaction|
  transaction.get("foo")
  transaction.del("bar")
end
```

This API allow multiple threads to build pipelines concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
…lock)

The new favored API is:

```ruby
redis.multi do |transaction|
  transaction.get("foo")
  transaction.del("bar")
end
```

This API allow multiple threads to build transactions concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
…lock)

The new favored API is:

```ruby
redis.multi do |transaction|
  transaction.get("foo")
  transaction.del("bar")
end
```

This API allow multiple threads to build transactions concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
…lock)

The new favored API is:

```ruby
redis.multi do |transaction|
  transaction.get("foo")
  transaction.del("bar")
end
```

This API allow multiple threads to build transactions concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
…lock)

The new favored API is:

```ruby
redis.multi do |transaction|
  transaction.get("foo")
  transaction.del("bar")
end
```

This API allow multiple threads to build transactions concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
…lock)

The new favored API is:

```ruby
redis.multi do |transaction|
  transaction.get("foo")
  transaction.del("bar")
end
```

This API allow multiple threads to build transactions concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
…lock)

The new favored API is:

```ruby
redis.multi do |transaction|
  transaction.get("foo")
  transaction.del("bar")
end
```

This API allow multiple threads to build transactions concurrently on the same
connection, and is more friendly to Fiber based concurrency.

Fix: redis#1057
casperisfine pushed a commit to casperisfine/redis-rb that referenced this pull request Jan 21, 2022
…lock)

The new favored API is:

```ruby
redis.multi do |transaction|
  transaction.get("foo")
  transaction.del("bar")
end
```

This API allow multiple threads to build transactions concurrently on the same
connection, and is more friendly to Fiber based concurrency.

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

Successfully merging this pull request may close these issues.

2 participants