feat(bindings/ruby): support layers #5874
Conversation
github-actions
bot
added
the
releases-note/feat
erickguan
force-pushed
the
ruby-binding-layers
branch
from
63a4b00 to
b70df7f
Compare
| @@ -103,4 +103,10 @@ class OpenDalTest < ActiveSupport::TestCase | |||
|
|
|||
| io.close | |||
| end | |||
|
|
|||
| test "layer applies a layer" do | |||
| @op.layer(OpenDAL::RetryLayer.new) | |||
There was a problem hiding this comment.
I'm guessing layer is not a widely accepted concept in ruby? Do we need to use middleware or something?
There was a problem hiding this comment.
Indeed, you are bringing a sensible pattern for Ruby users. Most libraries choose "middleware" as a name. We can start with "middleware".
The convention brings an annoying problem to solve - curating documentation.
bindings/ruby/src/layers.rs
Outdated
| // Magnus sets the correct class via `.class_for()` using the enum variant. | ||
| // Rust tracks the exact variant in memory; Ruby doesn’t need to know. | ||
| #[magnus::wrap(class = "OpenDAL::Layer", free_immediately, size)] | ||
| pub enum Layer { |
There was a problem hiding this comment.
Can we avoid wrapping them in an enum like this?
There was a problem hiding this comment.
Yes, with some vtables.
I have one last design question to solve - if a base class is useful or not. I haven't thought through it yet.
I will come back to this so that I can make a few packages with different services and classes.
There was a problem hiding this comment.
I've added a new implementation with a few key changes:
We're now using duck typing to apply layers instead of relying on Magnus's strong typing. This removes the need for the wrapping enum.
I chose not to introduce a trait for accepting middleware in the operator. Traits don’t integrate well with Magnus’s type conversion system—we’d end up writing conversion code for each middleware, which would reduce flexibility and increase complexity.
I removed the base class, as shared behavior seems unlikely at this point. There’s no strong need to enforce inheritance now, and we can always introduce a base class later if it becomes useful.
bindings/ruby/src/layers.rs
Outdated
| #[magnus(class = "OpenDAL::ThrottleLayer")] | ||
| Throttle(Arc<Mutex<ocore::layers::ThrottleLayer>>), | ||
| #[magnus(class = "OpenDAL::TimeoutLayer")] | ||
| Timeout(Arc<Mutex<ocore::layers::TimeoutLayer>>), |
There was a problem hiding this comment.
I somehow wish we can build seperate ruby package like opendal-layer-timeout and allow users to use them like native ruby extentions or middleware.
This also can avoid compiling everything together.
There was a problem hiding this comment.
Exactly, this is relevant to your comment on enum Layer. I reply after your comment.
This also can avoid compiling everything together.
Totally. I wouldn't want symbols from other services when only using S3 either.
Also various improvements over #inspect methods to be friendly within interactive consoles.
Also simplifies the implementation
erickguan
force-pushed
the
ruby-binding-layers
branch
from
29b55d4 to
1808b95
Compare
dosubot
bot
added
the
size:L
Which issue does this PR close?
Part of #5227.
Rationale for this change
What changes are included in this PR?
Operator#layer.#inspectimplementationAre there any user-facing changes?