Skip to content

Comments

feat(bindings/ruby): support layers#5874

Merged
erickguan merged 5 commits intoapache:mainfrom
erickguan:ruby-binding-layers
Apr 5, 2025
Merged

feat(bindings/ruby): support layers#5874
erickguan merged 5 commits intoapache:mainfrom
erickguan:ruby-binding-layers

Conversation

@erickguan
Copy link
Member

Which issue does this PR close?

Part of #5227.

Rationale for this change

What changes are included in this PR?

  1. 4 Layers and supports for Operator#layer.
  2. Documentation update
  3. A few #inspect implementation

Are there any user-facing changes?

@erickguan erickguan requested a review from PsiACE as a code owner March 24, 2025 20:29
@github-actions github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Mar 24, 2025
Copy link
Member

@PsiACE PsiACE left a comment

Choose a reason for hiding this comment

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

lgtm. It's nice to see that the ruby binding is starting to support layers, and I hope there will be some users willing to use it in their projects.

end

test "layer applies a layer" do
@op.layer(OpenDAL::RetryLayer.new)
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing layer is not a widely accepted concept in ruby? Do we need to use middleware or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid wrapping them in an enum like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

#[magnus(class = "OpenDAL::ThrottleLayer")]
Throttle(Arc<Mutex<ocore::layers::ThrottleLayer>>),
#[magnus(class = "OpenDAL::TimeoutLayer")]
Timeout(Arc<Mutex<ocore::layers::TimeoutLayer>>),
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@erickguan erickguan requested a review from Xuanwo March 29, 2025 15:30
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 5, 2025
@erickguan erickguan merged commit 6738df2 into apache:main Apr 5, 2025
34 checks passed
@erickguan erickguan deleted the ruby-binding-layers branch April 5, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants