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.

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

feat: support the flattening syntax for supervising #1386

Merged
merged 8 commits into from
Jul 27, 2024

Conversation

Roiocam
Copy link
Member

@Roiocam Roiocam commented Jul 3, 2024

MOTIVATION

Continue #680

The original api wasn't flatening.

Behaviors.supervise(
        Behaviors.supervise(behavior)
            .onFailure(IllegalStateException.class, SupervisorStrategy.restart()))
    .onFailure(IllegalArgumentException.class, SupervisorStrategy.stop());

RELATED WORK

  • scaladsl
  • javadsl
  • documentation
  • made unit test passed
  • binary compatibility

@@ -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))
Copy link
Member Author

Choose a reason for hiding this comment

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

The scala test migration is more than this, I will open a sperate PR for this.

@pjfanning pjfanning added this to the 1.1.0-M2 milestone Jul 3, 2024
@Roiocam Roiocam marked this pull request as ready for review July 5, 2024 01:15
Comment on lines 1 to 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")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I has been fix these binary compatibility via some duplicate code. would you take a look?

@Roiocam Roiocam force-pushed the flat-syntax-supervise branch 2 times, most recently from 6265b0f to facc0c8 Compare July 8, 2024 14:03
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

seems like a nice syntax

Comment on lines +19 to +20
ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.javadsl.Behaviors#Supervise.onFailure")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.pekko.actor.typed.scaladsl.Behaviors#Supervise.onFailure")
Copy link
Member

Choose a reason for hiding this comment

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

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.

# 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")
Copy link
Member

Choose a reason for hiding this comment

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

This one as well (checked with javap)

@Roiocam
Copy link
Member Author

Roiocam commented Jul 27, 2024

Thanks @raboof, let we merged it.

@Roiocam Roiocam merged commit 9885a58 into apache:main Jul 27, 2024
18 checks passed
@Roiocam Roiocam deleted the flat-syntax-supervise branch July 27, 2024 13:35
@pjfanning pjfanning added the release notes Need to release note label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes Need to release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants