You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "He-Pin (via GitHub)" <gi...@apache.org> on 2024/01/19 12:30:21 UTC
[PR] chore: Clean up some duplicated code. [incubator-pekko]
He-Pin opened a new pull request, #982:
URL: https://github.com/apache/incubator-pekko/pull/982
Motivation：
I found these code is a little duplicated.
Result:
Simpler code.
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460289288
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,12 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
- case Supervision.Restart => if (!hasBeenPulled(in)) pull(in)
+ case Supervision.Stop => failStage(ex)
+ case _ =>
+ // The !hasBeenPulled(in) check is not required here since it
Review Comment:
As a maintainer of Pekko, I have permission to add commits onto branches of other people's PR. This is my commit that I added now.
I will approve PR and squash + merge it, thanks for work!
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1459919341
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
Review Comment:
Why? the Map and Filter don't have this either.
I would like to add that for in `MapConcat` operator which explain why a `!hasBeenPulled(in)` test is needed.
I think the origin reason about why this `!hasBeenPulled(in)` test is because that method is a abstract class and so that exception can happen when onPush/onPull, so it have to be guard for extending, and reduce the sub class' burden. that's not the case 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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1459998741
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
Review Comment:
> Why? the Map and Filter don't have this either.
We should add the comments there as well then. Its more that when other people that are not so familiar with Akka/Pekko read the code they get a better understanding of why these exceptions are there.
Akka's problem historically is that a lot of the contributions were done within Lightbend and so there is a lot of inhouse knowledge that wasn't visible to outside contributors.
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460237067
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
Review Comment:
https://pekko.apache.org/docs/pekko/current/stream/stream-customize.html
It's detailly described 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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460289557
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,12 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
- case Supervision.Restart => if (!hasBeenPulled(in)) pull(in)
+ case Supervision.Stop => failStage(ex)
+ case _ =>
+ // The !hasBeenPulled(in) check is not required here since it
Review Comment:
I see, hah, The chinese networking to github is very unstable, that's why I hate changing and changing, sometime I can't pulling pull too, both via ssh and https, and VPN is illegal
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1458970998
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
- case Supervision.Restart => if (!hasBeenPulled(in)) pull(in)
Review Comment:
can you explain the change? - does `pull(in)` automatically check `!hasBeenPulled(in)`?
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460289557
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,12 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
- case Supervision.Restart => if (!hasBeenPulled(in)) pull(in)
+ case Supervision.Stop => failStage(ex)
+ case _ =>
+ // The !hasBeenPulled(in) check is not required here since it
Review Comment:
I see, hah, The chinese networking to github is very unstable, that's why I hate changing and changing, sometime I can't pulling pull too, both via ssh and https
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#issuecomment-1901243514
@mdedetrich @pjfanning I can not push anycode now, GFW blocked my network.
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460285296
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
Review Comment:
@He-Pin I was away from laptop, ill just add a commit with comment onto your branch and then merge it.
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1459044300
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
- case Supervision.Restart => if (!hasBeenPulled(in)) pull(in)
Review Comment:
There is no need to check the `(!hasBeenPulled(in))` , because there is no additional `pull(in)`. these method was copied from the old code, where, an exception maybe throw during the `preStart` but the upstream has been pulled. but here, as the `collect` operator never do some pulling or any other things which can throws after a `pull(in)`, so I think it would be better to just remove those duplicated check.
That's why I remove it.
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460287618
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,12 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
- case Supervision.Restart => if (!hasBeenPulled(in)) pull(in)
+ case Supervision.Stop => failStage(ex)
+ case _ =>
+ // The !hasBeenPulled(in) check is not required here since it
Review Comment:
@He-Pin Is this comment correct?
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460238380
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
Review Comment:
@mdedetrich Please unlock this, I'm not pay for everything, thanks, I just a volunteer, and this kind of information is well covered in the doc, this is code, not a book, and the Chinese GFW make everything harder, if you like to detail comment thing, I'm very glad to review.
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1459878784
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
Review Comment:
Can we add a comment here explaining why `!hasBeenPulled(in)` is not necessary.
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460238380
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
Review Comment:
@mdedetrich Please unlock this, I'm not pay for everything, thanks, I just a volunteer, and this kind of information is well covered in the doc, this is code, not a book.
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460288967
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,12 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
- case Supervision.Restart => if (!hasBeenPulled(in)) pull(in)
+ case Supervision.Stop => failStage(ex)
+ case _ =>
+ // The !hasBeenPulled(in) check is not required here since it
Review Comment:
Is true, but where this comments come from?
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460174581
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,8 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
Review Comment:
I can't push any code because gfw blocked me, Maybe I can use a web ide?
How about we do that in a separate pr or you take care about it?
--
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
Re: [PR] chore: Clean up some duplicated code. [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #982:
URL: https://github.com/apache/incubator-pekko/pull/982#discussion_r1460288967
##########
stream/src/main/scala/org/apache/pekko/stream/impl/fusing/Ops.scala:
##########
@@ -267,9 +266,12 @@ private[stream] object Collect {
} catch {
case NonFatal(ex) =>
decider(ex) match {
- case Supervision.Stop => failStage(ex)
- case Supervision.Resume => if (!hasBeenPulled(in)) pull(in)
- case Supervision.Restart => if (!hasBeenPulled(in)) pull(in)
+ case Supervision.Stop => failStage(ex)
+ case _ =>
+ // The !hasBeenPulled(in) check is not required here since it
Review Comment:
Is true, but where this comment comes from, I didn't remember...?
--
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