You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by GitBox <gi...@apache.org> on 2022/11/16 02:58:59 UTC
[GitHub] [incubator-pekko] He-Pin opened a new pull request, #48: =str Add LazyFutureSource to avoid sub materialization.
He-Pin opened a new pull request, #48:
URL: https://github.com/apache/incubator-pekko/pull/48
refs: https://github.com/apache/incubator-pekko/issues/46
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1279705548
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/LazyFutureSource.scala:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
Review Comment:
I think this is the wrong header.
@He-Pin did you write this code yourself or did you copy it from Akka. If you wrote it and did an Akka PR, you still own the copyright on the code.
I think the code should have an ASL header like this file:
https://github.com/apache/incubator-pekko/blob/main/project/AddMetaInfLicenseFiles.scala
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1279360053
##########
stream/src/main/scala/org/apache/pekko/stream/scaladsl/Source.scala:
##########
@@ -13,16 +13,15 @@ import scala.compat.java8.FutureConverters._
import scala.concurrent.{ Future, Promise }
import scala.concurrent.duration.FiniteDuration
-import org.reactivestreams.{ Publisher, Subscriber }
-
import org.apache.pekko
+import org.reactivestreams.{ Publisher, Subscriber }
Review Comment:
@pjfanning I will do a sortImports when I back to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich closed pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich closed pull request #48: =str Add LazyFutureSource to avoid sub materialization.
URL: https://github.com/apache/incubator-pekko/pull/48
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#issuecomment-1657846099
No worries its not urgent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1279337522
##########
stream/src/main/scala/org/apache/pekko/stream/scaladsl/Source.scala:
##########
@@ -13,16 +13,15 @@ import scala.compat.java8.FutureConverters._
import scala.concurrent.{ Future, Promise }
import scala.concurrent.duration.FiniteDuration
-import org.reactivestreams.{ Publisher, Subscriber }
-
import org.apache.pekko
+import org.reactivestreams.{ Publisher, Subscriber }
Review Comment:
can you move the reactivestreams import away from the pekko ones?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1279718017
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/LazyFutureSource.scala:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
Review Comment:
I copied it from another file, will update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#issuecomment-1332389616
@He-Pin You need to rebase this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1281619301
##########
.scalafix.conf:
##########
@@ -47,4 +47,5 @@ SortImports.blocks = [
"*",
"com.sun."
"org.apache.pekko."
+ "org.reactivestreams."
]
Review Comment:
nit: Can you add a newline here
##########
.scalafix.conf:
##########
@@ -47,4 +47,5 @@ SortImports.blocks = [
"*",
"com.sun."
"org.apache.pekko."
+ "org.reactivestreams."
]
Review Comment:
nit: Can you add a newline here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1279723361
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/LazyFutureSource.scala:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
Review Comment:
that special header saying `derived from Akka` is only for code that we took from Akka
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1278880965
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/LazyFutureSource.scala:
##########
@@ -0,0 +1,66 @@
+/*
Review Comment:
This header file is a slightly different variant compared to what else is in Pekko, see https://github.com/apache/incubator-pekko/blob/bd9ff4da5bb59234c39af8916fb7a06caa755052/project/AddMetaInfLicenseFiles.scala#L1-L16 for the correct version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin merged pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin merged PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#issuecomment-1657845367
@mdedetrich I will find some time to update these PR, but will afterwork, busy days, maybe this weekend.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1279706393
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/LazyFutureSource.scala:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
+ */
+
+package org.apache.pekko.stream.impl.fusing
+import scala.concurrent.Future
Review Comment:
can you leave a blank line before the imports?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#issuecomment-1657805114
@He-Pin Do you want to look into this now that we are on 1.1.x series (i.e. rebase on current main)? Also if possible and easy to do so would it be possible to add a benchmark in https://github.com/apache/incubator-pekko/tree/b0fdac259bd57fdd481483f3fe9a7aec6e1ff38a/bench-jmh/src/main/scala/org/apache/pekko/stream ? While I am sure that you version has better performance, the adding of the benchmark is to prevent any potential future performance regressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1281617443
##########
.scalafix.conf:
##########
@@ -47,4 +47,5 @@ SortImports.blocks = [
"*",
"com.sun."
"org.apache.pekko."
+ "org.reactivestreams."
]
Review Comment:
nit: Can you add a newline here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #48: =str Add LazyFutureSource to avoid sub materialization.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #48:
URL: https://github.com/apache/incubator-pekko/pull/48#discussion_r1279705548
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/LazyFutureSource.scala:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, which was derived from Akka.
Review Comment:
I think this is the wrong header.
@He-Pin did you write this code yourself or did you copy it from Akka? If you wrote it and did an Akka PR, you still own the copyright on the code.
I think the code should have an ASL header like this file:
https://github.com/apache/incubator-pekko/blob/main/project/AddMetaInfLicenseFiles.scala
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org