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.

Sign up for GitHub

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

Jump to bottom

ByteChannelContentSource #11910

Merged
merged 14 commits into from
Jun 24, 2024
Merged

ByteChannelContentSource #11910

merged 14 commits into from
Jun 24, 2024

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jun 12, 2024

This PR introduces a ByteChannelContentSource which is needed for the spring framework integration.

It also introduces a few minor utilities:

  • a ByteBufferPool.Sized class that is a ByteBufferPool.Wrapper with a configured size and type so a no args acquire() method can be called. This avoid the need to many classes to have a three fields: pool, size and direct and also can avoid constructors/methods with size and direct
  • an ExceptionUtil.run(Runnable, Consumer<Throwable> failure) that is a simple try catch and avoids many private methods being created to do just that.

@gregw gregw requested review from sbordet and lorban June 12, 2024 05:14
@lachlan-roberts
Copy link
Contributor

@gregw I looked at the ContentWriterIteratingCallback from the spring PR, and it looks like we already have something very similar, org.eclipse.jetty.io.IOResources.PathToSinkCopier which is used from IOResources#copy(Resource, Content.Sink, ByteBufferPool, int, boolean, long, long, Callback).

So considering we already have this do we really need the ByteChannelContentSource at all?

lorban
lorban requested changes Jun 12, 2024
View reviewed changes
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteChannelContentSource.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/InputStreamContentSource.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/PathContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/PathContentSource.java Show resolved Hide resolved
jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ContentSourceTest.java Outdated Show resolved Hide resolved
@gregw gregw requested a review from lorban June 12, 2024 11:53
lorban
lorban previously approved these changes Jun 17, 2024
View reviewed changes
sbordet
sbordet requested changes Jun 17, 2024
View reviewed changes
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/PathRequestContent.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteChannelContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteChannelContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteChannelContentSource.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/InputStreamContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/PathContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/internal/ContentCopierTest.java Outdated Show resolved Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ExceptionUtil.java Outdated Show resolved Hide resolved
@gregw gregw requested review from lorban and sbordet June 18, 2024 11:40
sbordet
sbordet previously requested changes Jun 18, 2024
View reviewed changes
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteChannelContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteChannelContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/PathRequestContent.java Outdated Show resolved Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/PathRequestContent.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteChannelContentSource.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ByteChannelContentSource.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/InputStreamContentSource.java Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/PathContentSource.java Outdated Show resolved Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java Show resolved Hide resolved
@gregw
updates from review
a4ac020 
Reverted PathContentSource to previous implementation, but with deprecated methods
Simplified ByteChannelContentSource
more rewind tests.
@gregw gregw dismissed sbordet’s stale review June 18, 2024 23:25

@sbordet is on vacation. @lorban will review

@gregw
Copy link
Contributor Author

gregw commented Jun 18, 2024

@lorban I have met @sbordet's review as much as I can. Can you please re-review this ASAP!

@gregw
updates from review
bccdf83 
Reverted PathContentSource to previous implementation, but with deprecated methods
Simplified ByteChannelContentSource
more rewind tests.
@gregw
Copy link
Contributor Author

gregw commented Jun 19, 2024

@lachlan-roberts can you review this in light of @sbordet being on vacation. Can you check if I have addressed all his points sufficiently.
Note that @lorban has previously given this the green tick, before the changes requested by @sbordet (but didn't re-review yesterday).

@gregw gregw merged commit 4f4bec0 into jetty-12.0.x Jun 24, 2024
10 checks passed
@gregw gregw deleted the jetty-12.0.x-ByteChannelContentSource branch June 24, 2024 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lachlan-roberts lachlan-roberts lachlan-roberts approved these changes

@sbordet sbordet sbordet left review comments

@lorban lorban Awaiting requested review from lorban

Assignees
No one assigned
Labels
High Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gregw @lachlan-roberts @sbordet @lorban