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

feat: Add AutoCloseable shortcut on mapWithResource #1053

Merged
merged 9 commits into from
Feb 1, 2024
Merged

feat: Add AutoCloseable shortcut on mapWithResource #1053

merged 9 commits into from
Feb 1, 2024

Conversation

injae-kim
Copy link
Contributor

Motivation

  • Pekko has mapwithResource now, would be better to support AutoCloseable in Java/Scala dsl too.

Modification

  • Add AutoCloseable shortcut on mapWithResource

Result

@injae-kim

This comment was marked as resolved.

Sign in to view
@injae-kim

This comment was marked as resolved.

Sign in to view
@He-Pin
Copy link
Member

He-Pin commented Jan 27, 2024

Thank you.

  1. We will need an apache cla before we can merge this

@injae-kim injae-kim force-pushed the mapWithResource-autoCloseable branch from fa4ebb8 to 57cc873 Compare January 27, 2024 10:36
@injae-kim

This comment was marked as resolved.

Sign in to view
@He-Pin
Copy link
Member

He-Pin commented Jan 27, 2024

Can't wait to use this on may daily job

@injae-kim
Copy link
Contributor Author

injae-kim commented Jan 27, 2024 •
edited
Loading

I submitted CLA agreement PDF by email~ I'll share you when I got reply.

pjfanning
pjfanning previously requested changes Jan 27, 2024
View reviewed changes
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

raised an issue - I think the tests should ensure that the close runs after the map function.

@injae-kim injae-kim force-pushed the mapWithResource-autoCloseable branch from 57cc873 to 31f0dc9 Compare January 27, 2024 11:11
@injae-kim
Copy link
Contributor Author

Thanks for quick review! I'm struggling with build failure on my local env,
so I'll update this PR based on your review after build&run test on my local env~! thanks! 🙇

@He-Pin
Copy link
Member

He-Pin commented Jan 27, 2024

@injae-kim I think you can update the mapWithResource.md too, you can do a local preview with sbt paradoxBrowse ,have a nice weekend.

@He-Pin
Copy link
Member

He-Pin commented Jan 27, 2024 •
edited
Loading

@injae-kim In very good shape, just need another update, thanks.
BTW, it's too late in both our countries, we can do it tomorrow, and good night:)

@injae-kim
Copy link
Contributor Author

injae-kim commented Jan 27, 2024 •
edited
Loading

Now I can build&run test on my local 😃 I also got email that my ICLA is received well~!

Thanks a lot for your help!

@injae-kim injae-kim force-pushed the mapWithResource-autoCloseable branch 2 times, most recently from 87192f8 to f2e77c3 Compare January 27, 2024 16:43
He-Pin
He-Pin reviewed Jan 27, 2024
View reviewed changes
stream-tests/src/test/java/org/apache/pekko/stream/javadsl/SourceTest.java
public void mustBeAbleToUseMapWithAutoCloseableResource() {
final TestKit probe = new TestKit(system);
final AtomicInteger closed = new AtomicInteger();
Source.from(Arrays.asList("1", "2", "3"))
Copy link
Member

Choose a reason for hiding this comment

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

I want add something like Flux.elements for java dsl...

@injae-kim injae-kim force-pushed the mapWithResource-autoCloseable branch from f2e77c3 to 63d191f Compare January 27, 2024 16:53
@He-Pin
Copy link
Member

He-Pin commented Jan 28, 2024 •
edited
Loading

Yeah, that's true, But we are not using Scala 2.13, maybe a conditional compiling can help us add that using to 2.13? I don't know when you plan to drop the 2.12 support:), seems like 2 years later.

and I checked the fs2 :

  /** Converts the supplied [[java.lang.AutoCloseable]] into a singleton stream. */
  def fromAutoCloseable[F[_]: Sync, O <: AutoCloseable](fo: F[O]): Stream[F, O] =
    Stream.resource(Resource.fromAutoCloseable(fo))

@He-Pin He-Pin modified the milestones: 1.1.0-M1, 1.1.0-M2 Jan 28, 2024
@He-Pin He-Pin modified the milestones: 1.1.0-M2, 1.1.0-M1 Jan 30, 2024
@He-Pin
Copy link
Member

He-Pin commented Jan 30, 2024

@injae-kim Thank you ,seems you will just need a final update and then looks good to me. as @mdedetrich , I think that's will not be a problem, because Even FS2 supports AutoCloseable.

He-Pin
He-Pin approved these changes Jan 30, 2024
View reviewed changes
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin
Copy link
Member

He-Pin commented Jan 30, 2024

@Roiocam ping for additional CR, as a Java CRUD guy, I love this.

Roiocam
Roiocam approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks

docs/src/main/paradox/stream/operators/Source-or-Flow/mapWithResource.md Outdated Show resolved Hide resolved
docs/src/main/paradox/stream/operators/Source-or-Flow/mapWithResource.md Show resolved Hide resolved
@injae-kim injae-kim force-pushed the mapWithResource-autoCloseable branch from 67a1c57 to 7200322 Compare January 31, 2024 11:30
@He-Pin
Copy link
Member

He-Pin commented Jan 31, 2024

@pjfanning Would you like give this another round review?

@pjfanning pjfanning dismissed their stale review January 31, 2024 12:55

old review - I don't have time this afternoon to re-review

@He-Pin He-Pin requested a review from jxnu-liguobin January 31, 2024 16:31
jxnu-liguobin
jxnu-liguobin reviewed Feb 1, 2024
View reviewed changes
stream/src/main/scala/org/apache/pekko/stream/javadsl/Source.scala Outdated
@@ -2541,6 +2541,45 @@ final class Source[Out, Mat](delegate: scaladsl.Source[Out, Mat]) extends Graph[
(resource, out) => f(resource, out),
resource => close.apply(resource).toScala))

/**
* Transform each stream element with the help of a [[AutoCloseable]] resource and close it when the stream finishes or fails.
Copy link
Member

Choose a reason for hiding this comment

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

an

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ updated. oh thanks for catching!

@He-Pin
Copy link
Member

He-Pin commented Feb 1, 2024

@injae-kim Good afternoon, seems only the last typo need to change, do you have any further improvement, thanks.

pjfanning
pjfanning approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin
Copy link
Member

He-Pin commented Feb 1, 2024

Thank you @injae-kim

@He-Pin He-Pin merged commit 7de1fb2 into apache:main Feb 1, 2024
17 of 18 checks passed
@injae-kim
Copy link
Contributor Author

Thank you to all reviewers! without your kind helps, I wouldn't have created this PR :)

I'll continue to contribute on pekko with another PR 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@He-Pin He-Pin He-Pin approved these changes

@jxnu-liguobin jxnu-liguobin jxnu-liguobin approved these changes

@Roiocam Roiocam Roiocam approved these changes

@pjfanning pjfanning pjfanning approved these changes

Assignees
No one assigned
Labels
t:stream Pekko Streams
Projects
None yet
Milestone
1.1.0-M1
Development

Successfully merging this pull request may close these issues.

Feature request: AutoCloseable shortcuts
6 participants
@injae-kim @He-Pin @pjfanning @mdedetrich @Roiocam @jxnu-liguobin