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

Introduce 'MemberDowned' member event #25854

Merged
merged 3 commits into from Nov 5, 2018

Conversation

Projects
None yet
5 participants
@raboof
Member

raboof commented Nov 2, 2018

Compatiblity note: MemberEvent is a sealed trait, so it is debatable whether
it is acceptable to introduce a new member.

Introduce 'MemberDowned' member event
Compatiblity note: MemberEvent is a sealed trait, so it is debatable whether
it is acceptable to introduce a new member.

@akka-ci akka-ci added the validating label Nov 2, 2018

@@ -317,6 +317,8 @@ object ClusterSingletonManager {
case state: CurrentClusterState handleInitial(state)
case MemberUp(m) add(m)
case MemberRemoved(m, _) remove(m)
case MemberDowned(m) if m.uniqueAddress != cluster.selfUniqueAddress
remove(m)

This comment has been minimized.

@patriknw

patriknw Nov 2, 2018

Member

That is a too scary change.
Note that MemberDowned will happen immediately since Down is an external/user action, while in contrast MemberExited is after convergence on leader and such.
Let's ignore MemberDowned in singleton.

@@ -247,6 +247,7 @@ final class ClusterSingletonProxy(singletonManagerPath: String, settings: Cluste
case state: CurrentClusterState handleInitial(state)
case MemberUp(m) add(m)
case MemberExited(m) remove(m)
case MemberDowned(m) remove(m)

This comment has been minimized.

@patriknw

patriknw Nov 2, 2018

Member

let's skip it here also

Show resolved Hide resolved akka-cluster/src/main/scala/akka/cluster/ClusterEvent.scala
@raboof

This comment has been minimized.

Member

raboof commented Nov 2, 2018

Agreed, will change things to be more conservative.

I guess we could make this more compatible by making MemberLeft a normal class (with manual 'case class sugar') and make MemberDowned a subclass of MemberLeft. Not sure if worth it?

@akka-ci akka-ci added needs-attention and removed validating labels Nov 2, 2018

@akka-ci

This comment has been minimized.

Collaborator

akka-ci commented Nov 2, 2018

Test FAILed.

raboof added some commits Nov 2, 2018

@akka-ci

This comment has been minimized.

Collaborator

akka-ci commented Nov 2, 2018

Test FAILed.

@patriknw

LGTM

@akka-ci akka-ci added tested and removed needs-attention labels Nov 2, 2018

@akka-ci

This comment has been minimized.

Collaborator

akka-ci commented Nov 2, 2018

Test PASSed.

@dwijnand

This comment has been minimized.

Member

dwijnand commented Nov 4, 2018

Compatiblity note: MemberEvent is a sealed trait, so it is debatable whether
it is acceptable to introduce a new member.

In lightbend/migration-manager#200 I argue that it's a form of binary breakage, one that MiMa should warn against.

But in Montreal Adriaan didn't really agree with me, while Seth did, so current it's 2 vs 1. 😉

@raboof

This comment has been minimized.

Member

raboof commented Nov 5, 2018

@dwijnand yes, I agree it'd be helpful if MiMa warned against it, but whether it is acceptable to ignore that warning is a harder question

@chbatey

chbatey approved these changes Nov 5, 2018

LGTM

@chbatey chbatey added this to the 2.5.18 milestone Nov 5, 2018

@chbatey chbatey merged commit 079aa46 into master Nov 5, 2018

4 checks passed

Jenkins PR Validation Test PASSed. 1299 tests run, 10 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@chbatey chbatey deleted the introduceMemberDownedEvent branch Nov 5, 2018

@dwijnand

This comment has been minimized.

Member

dwijnand commented Nov 6, 2018

Btw, I got paranoid that https://github.com/lightbend/reactive-lib/blob/master/akka-cluster-bootstrap/src/main/scala/com/lightbend/rp/akkaclusterbootstrap/ClusterStatusCheck.scala would break from this. But that's MemberStatus, not MemberEvent.

So I checked and it doesn't look like there is any exhaustive pattern matching on MemberEvent in https://github.com/lightbend/reactive-lib or https://github.com/akka/akka-management (checked by searching for MemberEvent and MemberRemoved in GitHub search, to find case branches for it).

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