You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pekko.apache.org by GitBox <gi...@apache.org> on 2022/11/03 03:10:22 UTC
[GitHub] [incubator-pekko] He-Pin opened a new pull request, #4: !str Logging error instead of failing the `keepAlive` operator.
He-Pin opened a new pull request, #4:
URL: https://github.com/apache/incubator-pekko/pull/4
refs: https://github.com/akka/akka/issues/30333
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
pjfanning commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1305388207
I don't seem to have the ability to create milestones. I created a label - 'not-for-initial-release' as a temp approach.
--
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
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012631598
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
I encounter this on some of my server, 77/257 nodes. and I think this warning will just show on the first *early* waking up？
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] danischroeter commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
danischroeter commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012612248
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
Review Comment:
we could also remove the entire comment and adapt log.warning to something like this:
```
log.warning(
s"Timer should have triggered only after deadline but now is $now and deadline was $nextDeadline diff $diff. (time running backwards?) Reschedule instead of emitting.")
```
This imho would be clearer to read the code and for the one stumbling upon this log...
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012631598
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
I encounter this on some of my servers, 77/257 nodes(in cloud). and I think this warning will just show on the first *early* waking up.
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] jrudolph commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
jrudolph commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012669094
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
> Maybe I'm wrong, but if you can go back in time with `System.nanoTime`, a lot of functionality breaks in projects.
Agreed. Though, monotonicity does not seem to be mentioned in its javadoc, so there's some wiggle room in the JVM to do something else. It seems that the JVM uses a monotonic clock source if available or fall back on whatever is available. See https://stackoverflow.com/questions/51344787/in-what-cases-clock-monotonic-might-not-be-available
The original ticket mentions two scenarios, a Kubernetes App on GCP and Windows. Let's ignore Windows. One issue might be that people use weird base images in their docker images (like alpine which has several edge cases people might not expect).
> My assumption is that this issue happens because the timer woke up _earlier_ for some reason. It's like when you do `Object.wait(1000)` ([link](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait(long))), but you're not guaranteed `1000 millis`, as it can wake up earlier, so you have to check, and sleep some more if needed.
Timers use the `LightArrayRevolverScheduler` which should not run tasks before the scheduled time (i.e. in the loop the current `nanoTime` is checked against the target time for a tick and otherwise more sleeping is added.
> If the timer simply wakes up earlier (the delay isn't guaranteed), then there's no need for the `log.warning`, since this is a happy path.
In any case, it's a path we can handle (and do after this change), so let's demote to INFO or even DEBUG.
--
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: commits-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pekko.apache.org
For additional commands, e-mail: commits-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012673644
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
> Agreed. Though, monotonicity does not seem to be mentioned in its javadoc, so there's some wiggle room in the JVM to do something else. It seems that the JVM uses a monotonic clock source if available or fall back on whatever is available. See https://stackoverflow.com/questions/51344787/in-what-cases-clock-monotonic-might-not-be-available
I would opt for at least updating the comment that this behaviour is due to the possibility of a non-monotonic clock (along with a reference to the stack overflow question). Will update review to reflect 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: commits-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pekko.apache.org
For additional commands, e-mail: commits-help@pekko.apache.org
[GitHub] [incubator-pekko] jrudolph commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
jrudolph commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012687269
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
> In this case it would help the user to get a WARN log. It should not be DEBUG since it's not expected to happen. And even for the people that stumbled upon its still rare.
No strong opinion on that but it seems a bit arbitrary to put that warning into this code especially, since here we can just fix it without any bad effect for the user (even though it might be a problem somewhere else).
--
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: commits-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pekko.apache.org
For additional commands, e-mail: commits-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1334760224
Yes, I will do all these this weekend. @mdedetrich
--
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
[GitHub] [incubator-pekko] He-Pin commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1305338991
@mdedetrich Create a milestone?
--
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
[GitHub] [incubator-pekko] danischroeter commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
danischroeter commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012609291
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
Review Comment:
if i understand correctly:
`// reschedule for negative time drift to ensure not triggered too early
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1665974490
@mdedetrich @pjfanning I have rebased 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
[GitHub] [incubator-pekko] danischroeter commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
danischroeter commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012637102
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
There seems to be no clear reason why this happens since ntp is guaranteed to be monotonic - but we do know that it happens. Therefore I find a warning log to be appropriate. Users should investigate the cause of it and maybe in the future we'll have a better way (or maybe a fixed jvm and this does not happen anymore...)
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1305340324
Sure, @pjfanning can do this I believe
--
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
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1284704564
##########
stream/src/main/scala/org/apache/pekko/stream/impl/Timers.scala:
##########
@@ -268,33 +268,29 @@ import pekko.stream.stage._
push(out, grab(in))
if (isClosed(in)) completeStage()
else pull(in)
- } else {
- val now = System.nanoTime()
- // Idle timeout triggered a while ago and we were just waiting for pull.
- // In the case of now == deadline, the deadline has not passed strictly, but scheduling another thunk
- // for that seems wasteful.
- if (now - nextDeadline >= 0) {
- nextDeadline = now + timeout.toNanos
- push(out, inject())
- } else
- scheduleOnce(GraphStageLogicTimer, FiniteDuration(nextDeadline - now, TimeUnit.NANOSECONDS))
- }
+ } else emitInjectedElementOrReschedule(onTimer = false)
}
- override protected def onTimer(timerKey: Any): Unit = {
+ private def emitInjectedElementOrReschedule(onTimer: Boolean): Unit = {
val now = System.nanoTime()
- // Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
- // than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
- s"Timer should have triggered only after deadline but now is $now and deadline was $nextDeadline diff ${now - nextDeadline}.")
- push(out, inject())
- nextDeadline = now + timeout.toNanos
+ val diff = now - nextDeadline
+ if (diff < 0) {
+ if (onTimer) {
+ // Clock may be non-monotonic, see https://stackoverflow.com/questions/51344787/in-what-cases-clock-monotonic-might-not-be-available
+ log.info(
+ s"Timer should have triggered only after deadline but now is $now and deadline was $nextDeadline diff $diff. (time running backwards?) Reschedule instead of emitting.")
+ }
+ scheduleOnce(GraphStageLogicTimer, FiniteDuration(-diff, TimeUnit.NANOSECONDS))
+ } else {
+ push(out, inject())
+ nextDeadline = now + timeout.toNanos
+ }
}
- }
- override def toString = "IdleTimer"
+ override protected def onTimer(timerKey: Any): Unit = emitInjectedElementOrReschedule(onTimer = true)
+ }
+ override def toString = "IdleInject"
Review Comment:
why change the `toString` ?
--
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
[GitHub] [incubator-pekko] He-Pin commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1666103952
> lgtm - seems ok to me if everyone else is happy with it
I encountered this once in production and have to do a reboot
--
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
[GitHub] [incubator-pekko] mdedetrich commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1305222955
Awesome, lets not merge this right now because its altering the behaviour of Pekko so it should be merged only into the 1.1.x branch when its created.
--
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
[GitHub] [incubator-pekko] danischroeter commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
danischroeter commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012717996
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
> just fix it without any bad effect for the user
-> debug
> might be a problem somewhere else
-> info or warn
I don't have a strong opinion here as well. I feel like we could consent on INFO.
--
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: commits-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pekko.apache.org
For additional commands, e-mail: commits-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1301913656
Thanks for the detail review, I will update tomorrow, house moving day:)
--
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: commits-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pekko.apache.org
For additional commands, e-mail: commits-help@pekko.apache.org
[GitHub] [incubator-pekko] danischroeter commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
danischroeter commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012609291
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
Review Comment:
if i understand correctly:
`// reschedule for negative time drift to ensure not triggered too early`
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] Claudenw commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
Claudenw commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1020102854
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
I would vote for INFO
--
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
[GitHub] [incubator-pekko] danischroeter commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
danischroeter commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012588061
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
Review Comment:
this comment also would need to be updated
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] alexandru commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
alexandru commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012614433
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
Kind of weird that this invariant doesn't hold.
I understand that NTP can influence `System.nanoTime`, but I thought that the primary reason to use `nanoTime` is that it's guaranteed to be monotonic, and whenever NTP synchronization happens, the clocks simply slow down. Maybe I'm wrong, but if you can go back in time with `System.nanoTime`, a lot of functionality breaks in projects.
My assumption is that this issue happens because the timer woke up *earlier* for some reason. It's like when you do `Object.wait(1000)` ([link](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait(long))), but you're not guaranteed `1000 millis`, as it can wake up earlier, so you have to check, and sleep some more if needed.
If the timer simply wakes up earlier (the delay isn't guaranteed), then there's no need for the `log.warning`, since this is a happy path. Just a suggestion — but warnings need to be actionable. E.g., what should the user do when it sees one? Should the user re-adjust some configuration (NTP maybe) or report a bug? In this case, I don't think there's anything the user can do.
And too many warnings in those logs leads to users ignoring warnings.
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012687517
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
> In this case it would help the user to get a WARN log. It should not be DEBUG since it's not expected to happen. And even for the people that stumbled upon its still rare.
I am on the fence about this. Initially I also thought this should be a `WARN` however if the user happens to be running a platform that doesn't support a monotonic clock then the warning will effectively be ignored because there isn't anything they can do apart from changing the platform.
And we all know how helpful ignored `WARN` messages are.
--
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: commits-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pekko.apache.org
For additional commands, e-mail: commits-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012631598
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
I encounter this on some of my servers, 77/257 nodes. and I think this warning will just show on the first *early* waking up？
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012595711
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
Review Comment:
Any suggestion?
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012631598
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
I encounter this on some of my servers, 77/257 nodes(in cloud). and I think this warning will just show on the first *early* waking up.
So you suggest we just remove the warning?
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1015297405
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -258,33 +258,29 @@ import akka.stream.stage._
push(out, grab(in))
if (isClosed(in)) completeStage()
else pull(in)
- } else {
- val now = System.nanoTime()
- // Idle timeout triggered a while ago and we were just waiting for pull.
- // In the case of now == deadline, the deadline has not passed strictly, but scheduling another thunk
- // for that seems wasteful.
- if (now - nextDeadline >= 0) {
- nextDeadline = now + timeout.toNanos
- push(out, inject())
- } else
- scheduleOnce(GraphStageLogicTimer, FiniteDuration(nextDeadline - now, TimeUnit.NANOSECONDS))
- }
+ } else emitInjectedElementOrReschedule(onTimer = false)
}
- override protected def onTimer(timerKey: Any): Unit = {
+ private def emitInjectedElementOrReschedule(onTimer: Boolean): Unit = {
val now = System.nanoTime()
- // Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
- // than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
- s"Timer should have triggered only after deadline but now is $now and deadline was $nextDeadline diff ${now - nextDeadline}.")
- push(out, inject())
- nextDeadline = now + timeout.toNanos
+ val diff = now - nextDeadline
+ if (diff < 0) {
+ if (onTimer) {
Review Comment:
only trigged by timer will do the logging
--
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
[GitHub] [incubator-pekko] He-Pin commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1305093981
Rebased on to current main
--
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
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1284716526
##########
stream/src/main/scala/org/apache/pekko/stream/impl/Timers.scala:
##########
@@ -268,33 +268,29 @@ import pekko.stream.stage._
push(out, grab(in))
if (isClosed(in)) completeStage()
else pull(in)
- } else {
- val now = System.nanoTime()
- // Idle timeout triggered a while ago and we were just waiting for pull.
- // In the case of now == deadline, the deadline has not passed strictly, but scheduling another thunk
- // for that seems wasteful.
- if (now - nextDeadline >= 0) {
- nextDeadline = now + timeout.toNanos
- push(out, inject())
- } else
- scheduleOnce(GraphStageLogicTimer, FiniteDuration(nextDeadline - now, TimeUnit.NANOSECONDS))
- }
+ } else emitInjectedElementOrReschedule(onTimer = false)
}
- override protected def onTimer(timerKey: Any): Unit = {
+ private def emitInjectedElementOrReschedule(onTimer: Boolean): Unit = {
val now = System.nanoTime()
- // Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
- // than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
- s"Timer should have triggered only after deadline but now is $now and deadline was $nextDeadline diff ${now - nextDeadline}.")
- push(out, inject())
- nextDeadline = now + timeout.toNanos
+ val diff = now - nextDeadline
+ if (diff < 0) {
+ if (onTimer) {
+ // Clock may be non-monotonic, see https://stackoverflow.com/questions/51344787/in-what-cases-clock-monotonic-might-not-be-available
+ log.info(
+ s"Timer should have triggered only after deadline but now is $now and deadline was $nextDeadline diff $diff. (time running backwards?) Reschedule instead of emitting.")
+ }
+ scheduleOnce(GraphStageLogicTimer, FiniteDuration(-diff, TimeUnit.NANOSECONDS))
+ } else {
+ push(out, inject())
+ nextDeadline = now + timeout.toNanos
+ }
}
- }
- override def toString = "IdleTimer"
+ override protected def onTimer(timerKey: Any): Unit = emitInjectedElementOrReschedule(onTimer = true)
+ }
+ override def toString = "IdleInject"
Review Comment:
It was missed, and with one will help for the stage debugging
--
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
[GitHub] [incubator-pekko] mdedetrich commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1332391776
@He-Pin You need to rebase 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
[GitHub] [incubator-pekko] alexandru commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
alexandru commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012614433
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
Kind of weird that this invariant doesn't hold.
I understand that NTP can influence `System.nanoTime`, but I thought that the primary reason to use `nanoTime` is that it's guaranteed to be monotonic, and whenever NTP synchronization happens, the clocks simply slow down. Maybe I'm wrong, but if you can go back in time with `System.nanoTime`, a lot of functionality breaks in projects.
My assumption is that this issue happens because the timer woke up *earlier* for some reason. It's like when you do `Object.wait(1000)` ([link](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#wait(long))), but you're not guaranteed `1000 millis`, as it can wake up earlier, so you have to check, and sleep some more if needed.
If the timer simply weaks up earlier (the delay isn't guaranteed), then there's no need for the `log.warning`, since this is a happy path. Just a suggestion — but warnings need to be actionable. E.g., what should the user do when it sees one? Should the user re-adjust some configuration (NTP maybe) or report a bug? In this case, I don't think there's anything the user can do.
And too many warnings in those logs leads to users ignoring warnings.
--
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: dev-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pekko.apache.org
For additional commands, e-mail: dev-help@pekko.apache.org
[GitHub] [incubator-pekko] danischroeter commented on a diff in pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
danischroeter commented on code in PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#discussion_r1012678036
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
> One issue might be that people use weird base images in their docker images (like alpine which has several edge cases people might not expect).
In this case it would help the user to get a WARN log. It should not be DEBUG since it's not expected to happen. And even for the people that stumbled upon its still rare.
##########
akka-stream/src/main/scala/akka/stream/impl/Timers.scala:
##########
@@ -275,11 +275,15 @@ import akka.stream.stage._
val now = System.nanoTime()
// Timer is reliably cancelled if a regular element arrives first. Scheduler rather schedules too late
// than too early so the deadline must have passed at this time.
- assert(
- now - nextDeadline >= 0,
Review Comment:
> One issue might be that people use weird base images in their docker images (like alpine which has several edge cases people might not expect).
In this case it would help the user to get a WARN log. It should not be DEBUG since it's not expected to happen. And even for the people that stumbled upon its still rare.
--
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: commits-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pekko.apache.org
For additional commands, e-mail: commits-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by GitBox <gi...@apache.org>.
He-Pin commented on PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4#issuecomment-1374735682
will close I think.
--
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
[GitHub] [incubator-pekko] He-Pin merged pull request #4: !str Logging error instead of failing the `keepAlive` operator.
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin merged PR #4:
URL: https://github.com/apache/incubator-pekko/pull/4
--
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