You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "Roiocam (via GitHub)" <gi...@apache.org> on 2024/07/03 16:19:46 UTC
[PR] feat: support the flattening syntax for supervising [pekko]
Roiocam opened a new pull request, #1386:
URL: https://github.com/apache/pekko/pull/1386
(no comment)
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1687496784
##########
actor-typed/src/main/mima-filters/1.0.x.backwards.excludes/issue-3413.excludes:
##########
@@ -0,0 +1,6 @@
+ProblemFilters.exclude[MissingFieldProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors.Supervise")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors$Supervise$")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors$Supervise")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors$Supervise")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors.supervise")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors.supervise")
Review Comment:
I has been fix these binary compatibility via some duplicate code. would you take a look?
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1668179795
##########
actor-typed/src/main/mima-filters/1.0.x.backwards.excludes/issue-3413.excludes:
##########
@@ -0,0 +1,6 @@
+ProblemFilters.exclude[MissingFieldProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors.Supervise")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors$Supervise$")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors$Supervise")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors$Supervise")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors.supervise")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors.supervise")
Review Comment:
> I really think we should not just add things to mima backwards excludes, but also add comments why the exclusions are justified.
I totally agree with that.
> In this case it looks, to me, like the mima errors were actually legitimate
Yes.
What should we do when we face such API changes?
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1664508504
##########
actor-typed-tests/src/test/scala/org/apache/pekko/actor/typed/SupervisionSpec.scala:
##########
@@ -844,7 +844,8 @@ class SupervisionSpec extends ScalaTestWithActorTestKit("""
"support nesting to handle different exceptions" in {
val probe = TestProbe[Event]("evt")
val behv = Behaviors
- .supervise(Behaviors.supervise(targetBehavior(probe.ref)).onFailure[Exc2](SupervisorStrategy.resume))
+ .supervise(targetBehavior(probe.ref))
Review Comment:
The scala test migration is more than this, I will open a sperate PR for 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
Re: [PR] feat: support the flattening syntax for supervising [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1664523062
##########
actor-typed/src/main/scala/org/apache/pekko/actor/typed/Behavior.scala:
##########
@@ -71,6 +69,43 @@ abstract class Behavior[T](private[pekko] val _tag: Int) { behavior =>
}
+/**
+ * TODO scala doc
+ */
+final class SuperviseBehavior[T] private[pekko] (
+ val wrapped: Behavior[T]) extends Behavior[T](BehaviorTags.SuperviseBehavior) {
+ private final val ThrowableClassTag = ClassTag(classOf[Throwable])
+
+ /** Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws. */
+ def onFailure[Thr <: Throwable](strategy: SupervisorStrategy)(
+ implicit tag: ClassTag[Thr] = ThrowableClassTag): SuperviseBehavior[T] = {
+ val effectiveTag = if (tag == ClassTag.Nothing) ThrowableClassTag else tag
+ new SuperviseBehavior[T](Supervisor(Behavior.validateAsInitial(wrapped), strategy)(effectiveTag))
+ }
+
+ /**
+ * Java API:
+ * Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws.
+ *
+ * Only exceptions of the given type (and their subclasses) will be handled by this supervision behavior.
+ */
+ def onFailure[Thr <: Throwable](clazz: Class[Thr], strategy: SupervisorStrategy): SuperviseBehavior[T] = {
+ onFailure(strategy)(ClassTag(clazz))
+ }
+
+ /**
+ * Java API:
+ * Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws.
+ *
+ * Only exceptions of the given type (and their subclasses) will be handled by this supervision behavior.
+ */
+ def onNotFatalFailure[Thr <: Throwable](strategy: SupervisorStrategy): SuperviseBehavior[T] = {
+ onFailure(classOf[Exception], strategy)
Review Comment:
Better with NonFatal apply？
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam merged PR #1386:
URL: https://github.com/apache/pekko/pull/1386
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1687568292
##########
actor-typed/src/main/mima-filters/1.0.x.backwards.excludes/issue-3413.excludes:
##########
@@ -0,0 +1,21 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Change the return type of `Behaviors.supervise` to support flattened supervision
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors#Supervise.onFailure")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors#Supervise.onFailure")
Review Comment:
So these are OK because the old signatures returned `Behavior`, and the new ones return `SuperviseBehavior`, and since `SuperviseBehavior extends Behavior` every `SuperviseBehavior` is a `Behavior`? Sounds reasonable.
##########
actor-typed/src/main/mima-filters/1.0.x.backwards.excludes/issue-3413.excludes:
##########
@@ -0,0 +1,21 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Change the return type of `Behaviors.supervise` to support flattened supervision
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors#Supervise.onFailure")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors#Supervise.onFailure")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors#Supervise.onFailure$extension")
Review Comment:
This one as well (checked with `javap`)
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1668164563
##########
actor-typed/src/main/mima-filters/1.0.x.backwards.excludes/issue-3413.excludes:
##########
@@ -0,0 +1,6 @@
+ProblemFilters.exclude[MissingFieldProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors.Supervise")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors$Supervise$")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors$Supervise")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors$Supervise")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors.supervise")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors.supervise")
Review Comment:
I really think we should not just add things to mima backwards excludes, but also add comments why the exclusions are justified.
In this case it looks, to me, like the mima errors were actually legitimate and this PR breaks our binary compatibility promises documented at https://pekko.apache.org/docs/pekko/current/common/binary-compatibility-rules.html and https://github.com/apache/pekko/blob/main/CONTRIBUTING.md#binary-compatibility
To make things explicit: suppose we have an application 'A' which uses a library 'L' which depends on Pekko 1.0.0 . The point of the binary compatibility rules is that it should be safe for 'A' to update to Pekko 1.1.0 without breaking.
In this case, if `L` contained:
```
val s: Behaviors.Supervise = Behaviors.supervise(SupervisedActor.create())
```
... AFAICS this code would throw a runtime exception when executed with any Pekko version that has the change in this PR, because `Behaviors.Supervise` no longer exists.
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1664507316
##########
actor-typed-tests/src/test/java/jdocs/org/apache/pekko/typed/RouterTest.java:
##########
@@ -133,7 +133,7 @@ static Behavior<Void> showPoolRouting() {
Routers.pool(
poolSize,
// make sure the workers are restarted if they fail
- Behaviors.supervise(Worker.create()).onFailure(SupervisorStrategy.restart()));
+ Behaviors.supervise(Worker.create()).onNotFatalFailure(SupervisorStrategy.restart()));
Review Comment:
This method signature is because the overload issue between java API and scala API
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1668200245
##########
actor-typed/src/main/mima-filters/1.0.x.backwards.excludes/issue-3413.excludes:
##########
@@ -0,0 +1,6 @@
+ProblemFilters.exclude[MissingFieldProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors.Supervise")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors$Supervise$")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors$Supervise")
+ProblemFilters.exclude[MissingClassProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors$Supervise")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors.supervise")
+ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors.supervise")
Review Comment:
> What should we do when we face such API changes?
It depends on the situation. I haven't looked closely, but in this case, it looks like the SuperviseBehaviour was introduced to reduce code duplication between the two `Behavior.Supervise` classes, is that correct? I wonder if we could restore binary compatibility while still avoiding code duplication by restoring the `Behavior.Supervise` classes, but having them `extends SuperviseBehaviour`. If that does not work, possibly we should just choose to accept some duplication to achieve compatibility.
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on PR #1386:
URL: https://github.com/apache/pekko/pull/1386#issuecomment-2254151529
Thanks @raboof, let we merged 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] feat: support the flattening syntax for supervising [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1664945162
##########
actor-typed/src/main/scala/org/apache/pekko/actor/typed/Behavior.scala:
##########
@@ -71,6 +69,43 @@ abstract class Behavior[T](private[pekko] val _tag: Int) { behavior =>
}
+/**
+ * TODO scala doc
+ */
+final class SuperviseBehavior[T] private[pekko] (
+ val wrapped: Behavior[T]) extends Behavior[T](BehaviorTags.SuperviseBehavior) {
+ private final val ThrowableClassTag = ClassTag(classOf[Throwable])
+
+ /** Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws. */
+ def onFailure[Thr <: Throwable](strategy: SupervisorStrategy)(
+ implicit tag: ClassTag[Thr] = ThrowableClassTag): SuperviseBehavior[T] = {
+ val effectiveTag = if (tag == ClassTag.Nothing) ThrowableClassTag else tag
+ new SuperviseBehavior[T](Supervisor(Behavior.validateAsInitial(wrapped), strategy)(effectiveTag))
+ }
+
+ /**
+ * Java API:
+ * Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws.
+ *
+ * Only exceptions of the given type (and their subclasses) will be handled by this supervision behavior.
+ */
+ def onFailure[Thr <: Throwable](clazz: Class[Thr], strategy: SupervisorStrategy): SuperviseBehavior[T] = {
+ onFailure(strategy)(ClassTag(clazz))
+ }
+
+ /**
+ * Java API:
+ * Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws.
+ *
+ * Only exceptions of the given type (and their subclasses) will be handled by this supervision behavior.
+ */
+ def onNotFatalFailure[Thr <: Throwable](strategy: SupervisorStrategy): SuperviseBehavior[T] = {
+ onFailure(classOf[Exception], strategy)
Review Comment:
What a about `onException`?
--
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] feat: support the flattening syntax for supervising [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #1386:
URL: https://github.com/apache/pekko/pull/1386#discussion_r1664559990
##########
actor-typed/src/main/scala/org/apache/pekko/actor/typed/Behavior.scala:
##########
@@ -71,6 +69,43 @@ abstract class Behavior[T](private[pekko] val _tag: Int) { behavior =>
}
+/**
+ * TODO scala doc
+ */
+final class SuperviseBehavior[T] private[pekko] (
+ val wrapped: Behavior[T]) extends Behavior[T](BehaviorTags.SuperviseBehavior) {
+ private final val ThrowableClassTag = ClassTag(classOf[Throwable])
+
+ /** Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws. */
+ def onFailure[Thr <: Throwable](strategy: SupervisorStrategy)(
+ implicit tag: ClassTag[Thr] = ThrowableClassTag): SuperviseBehavior[T] = {
+ val effectiveTag = if (tag == ClassTag.Nothing) ThrowableClassTag else tag
+ new SuperviseBehavior[T](Supervisor(Behavior.validateAsInitial(wrapped), strategy)(effectiveTag))
+ }
+
+ /**
+ * Java API:
+ * Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws.
+ *
+ * Only exceptions of the given type (and their subclasses) will be handled by this supervision behavior.
+ */
+ def onFailure[Thr <: Throwable](clazz: Class[Thr], strategy: SupervisorStrategy): SuperviseBehavior[T] = {
+ onFailure(strategy)(ClassTag(clazz))
+ }
+
+ /**
+ * Java API:
+ * Specify the [[SupervisorStrategy]] to be invoked when the wrapped behavior throws.
+ *
+ * Only exceptions of the given type (and their subclasses) will be handled by this supervision behavior.
+ */
+ def onNotFatalFailure[Thr <: Throwable](strategy: SupervisorStrategy): SuperviseBehavior[T] = {
+ onFailure(classOf[Exception], strategy)
Review Comment:
onNonFatalFailure would be better
--
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