He-Pin commented on code in PR #3676:
URL: https://github.com/apache/pekko/pull/3676#discussion_r4363247746
##########
remote/src/main/scala/org/apache/pekko/remote/artery/ArteryTransport.scala:
##########
@@ -788,8 +788,12 @@ private[remote] abstract class ArteryTransport(_system:
ExtendedActorSystem, _pr
case ShuttingDown => Future.successful(Done) // silence it
case exc: UidCollisionException =>
log.warning(exc.getMessage)
- // quarantine will not do much since handshake not completed, but for
good measures
- quarantine(peer.address, Some(peer.uid), exc.getMessage, harmless =
false)
+ // Quarantining the peer here would be ignored since the handshake is
not completed, and the
+ // uid belongs to the existing association. The envelope header only
carries the origin uid, so
+ // messages from the colliding peer cannot be told apart from the
existing association.
+ // Tell the colliding peer that it has been quarantined instead.
Control messages are not held
+ // back by the outbound handshake.
+ sendControl(peer.address, Quarantined(localAddress, peer))
Review Comment:
Minor: consider logging the action here, e.g. `log.info("Sending Quarantined
to colliding peer [{}]", peer)`, mirroring the regular quarantine path in
`Association.quarantine` ("Sending Quarantined to [{}]"). The receiver
currently logs only the collision warning, so its logs don't show that it told
the peer to step down; this would make it easier for operators to correlate the
receiver-side warning with the peer-side "Other node [...] quarantined this
node" warning.
##########
remote/src/test/scala/org/apache/pekko/remote/artery/RemoteConnectionSpec.scala:
##########
@@ -97,6 +115,7 @@ class RemoteConnectionSpec extends
ArteryMultiNodeSpec("pekko.remote.retry-gate-
// doesn't get through
localProbe.expectNoMessage()
expectCollisionWarning(collisionWarnings1)
+ expectQuarantinedBy(remote2Quarantined, localSystem)
Review Comment:
Optional nit: the healthy peer (`remoteSystem1`) is verified intact via
association state and echo round-trips, but nothing asserts it stays free of
`ThisActorSystemQuarantinedEvent`. Subscribing a probe on `remoteSystem1` and
doing a short `expectNoMessage` after this point would directly guard against
the `Quarantined` ever reaching the wrong node (the uid is shared, so this is
the main risk of keying on `peer.address`).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
