You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "nvollmar (via GitHub)" <gi...@apache.org> on 2024/02/21 14:05:26 UTC
[PR] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
nvollmar opened a new pull request, #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137
resolves #1136
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "jxnu-liguobin (via GitHub)" <gi...@apache.org>.
jxnu-liguobin commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1499425160
##########
remote/src/main/scala/org/apache/pekko/remote/FailureDetector.scala:
##########
@@ -40,6 +40,16 @@ trait FailureDetector {
}
+trait FailureDetectorWithAddress {
+
+ /**
+ * Address of observed host will be set after detector creation.
+ */
+ def address_=(addr: String): Unit
Review Comment:
Indeed, but it's OK and convenient in scala.
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1502253422
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -119,7 +117,8 @@ class PhiAccrualFailureDetector(
private val acceptableHeartbeatPauseMillis = acceptableHeartbeatPause.toMillis
// address below was introduced as a var because of binary compatibility constraints
Review Comment:
Can we prefix this comment with `NOTE` so that its easily searchable. Should also make a pekko issue to make this a non var targeting Pekko 2.0.x
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1497760441
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -14,15 +14,13 @@
package org.apache.pekko.remote
import java.util.concurrent.atomic.AtomicReference
-
import scala.annotation.tailrec
import scala.collection.immutable
import scala.concurrent.duration.Duration
import scala.concurrent.duration.FiniteDuration
-
import com.typesafe.config.Config
-
import org.apache.pekko
+import org.apache.pekko.annotation.InternalApi
Review Comment:
should be `import pekko.annotation.InternalApi`
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#issuecomment-1960370682
The use case would be to intercept or override the call to `recordInterval`
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#issuecomment-1961284954
Had to add a mima exclude for the removed `private[pekko]` method
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1499416393
##########
remote/src/main/scala/org/apache/pekko/remote/FailureDetector.scala:
##########
@@ -40,6 +40,16 @@ trait FailureDetector {
}
+trait FailureDetectorWithAddress {
+
+ /**
+ * Address of observed host will be set after detector creation.
+ */
+ def address_=(addr: String): Unit
Review Comment:
Address_ is not good name for Java developer
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1502253422
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -119,7 +117,8 @@ class PhiAccrualFailureDetector(
private val acceptableHeartbeatPauseMillis = acceptableHeartbeatPause.toMillis
// address below was introduced as a var because of binary compatibility constraints
Review Comment:
Can we prefix this comment with `NOTE` so that its easily searchable. Should also make a pekko issue to make this a non var targeting Pekko 2.0.x
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1502362146
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -119,7 +117,8 @@ class PhiAccrualFailureDetector(
private val acceptableHeartbeatPauseMillis = acceptableHeartbeatPause.toMillis
// address below was introduced as a var because of binary compatibility constraints
Review Comment:
#1148
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#issuecomment-1957248662
@pjfanning I can add more to the issue. But I’d consider this a rather trivial change.
There is no functional difference, the implementation is now open for Kanela to intercept the call to the new method to report that metric to Kamon. Or to extend this class and overwrite the method to get the interval.
Also the new trait makes it possible to implement a custom failure detector with the same functionality.
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1502255046
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -153,15 +151,7 @@ class PhiAccrualFailureDetector(
val interval = timestamp - latestTimestamp
// don't use the first heartbeat after failure for the history, since a long pause will skew the stats
if (isAvailable(timestamp)) {
Review Comment:
nit: Still a redundant single bracket here, i.e. should be
```scala
if (isAvailable(timestamp))
recordInterval(interval)
else
oldState.history
```
You can even put it on a single line
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -119,7 +117,8 @@ class PhiAccrualFailureDetector(
private val acceptableHeartbeatPauseMillis = acceptableHeartbeatPause.toMillis
// address below was introduced as a var because of binary compatibility constraints
Review Comment:
Can we prefix this comment with `NOTE` so that its easily searchable. Should also make a pekko issue to make this a non var targeting Pekko 2.0.x
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1498332046
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -153,16 +152,10 @@ class PhiAccrualFailureDetector(
val interval = timestamp - latestTimestamp
// don't use the first heartbeat after failure for the history, since a long pause will skew the stats
if (isAvailable(timestamp)) {
- if (interval >= (acceptableHeartbeatPauseMillis / 3 * 2) && eventStream.isDefined)
- eventStream.get.publish(
- Warning(
- this.toString,
- getClass,
- s"heartbeat interval is growing too large for address $address: $interval millis",
- Logging.emptyMDC,
- RemoteLogMarker.failureDetectorGrowing(address)))
- oldState.history :+ interval
- } else oldState.history
+ recordInterval(interval)
Review Comment:
nit: Can we remove the unnecessary `{` and `}` in the `if`/`else` statement so its closer to the older style.
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#issuecomment-1958825491
> Looks like link validator is broken because the url changed from `opensource.org/licenses/BSD-3-Clause` to `opensource.org/blog/license/bsd-3-clause`
This is unrelated to this PR so you can file an issue or fix it in another PR
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1499974924
##########
remote/src/main/scala/org/apache/pekko/remote/FailureDetector.scala:
##########
@@ -40,6 +40,16 @@ trait FailureDetector {
}
+trait FailureDetectorWithAddress {
+
+ /**
+ * Address of observed host will be set after detector creation.
+ */
+ def address_=(addr: String): Unit
Review Comment:
true, could also name it `setAddress`. Though I don’t really expect this to be a use case outside of very special circumstances.
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1498338404
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -153,16 +152,10 @@ class PhiAccrualFailureDetector(
val interval = timestamp - latestTimestamp
// don't use the first heartbeat after failure for the history, since a long pause will skew the stats
if (isAvailable(timestamp)) {
- if (interval >= (acceptableHeartbeatPauseMillis / 3 * 2) && eventStream.isDefined)
- eventStream.get.publish(
- Warning(
- this.toString,
- getClass,
- s"heartbeat interval is growing too large for address $address: $interval millis",
- Logging.emptyMDC,
- RemoteLogMarker.failureDetectorGrowing(address)))
- oldState.history :+ interval
- } else oldState.history
+ recordInterval(interval)
Review Comment:
will change it back
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1500487825
##########
remote/src/main/scala/org/apache/pekko/remote/FailureDetector.scala:
##########
@@ -40,6 +40,16 @@ trait FailureDetector {
}
+trait FailureDetectorWithAddress {
+
+ /**
+ * Address of observed host will be set after detector creation.
+ */
+ def address_=(addr: String): Unit
Review Comment:
updated it to make it more java friendly
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1502357823
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -153,15 +151,7 @@ class PhiAccrualFailureDetector(
val interval = timestamp - latestTimestamp
// don't use the first heartbeat after failure for the history, since a long pause will skew the stats
if (isAvailable(timestamp)) {
Review Comment:
I removed the redundant bracket, since it isn't an assignment I'd leave it on different lines for clarity
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#issuecomment-1958821217
Looks like link validator is broken because the url changed from `opensource.org/licenses/BSD-3-Clause` to `opensource.org/blog/license/bsd-3-clause`
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#issuecomment-1957071695
Would it be possible to write a fuller description to the issue? This is a non-trivial change to our code and we need to be able to release note 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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#issuecomment-1963584257
This would be ready to merge unless there are some objections/further remarks
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137#discussion_r1502358253
##########
remote/src/main/scala/org/apache/pekko/remote/PhiAccrualFailureDetector.scala:
##########
@@ -119,7 +117,8 @@ class PhiAccrualFailureDetector(
private val acceptableHeartbeatPauseMillis = acceptableHeartbeatPause.toMillis
// address below was introduced as a var because of binary compatibility constraints
Review Comment:
I'll create an issue for Pekko 2.0
--
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] Rework PhiAccrualFailureDetector to enable monitoring of interval [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar merged PR #1137:
URL: https://github.com/apache/incubator-pekko/pull/1137
--
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