You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "shunping (via GitHub)" <gi...@apache.org> on 2025/05/30 03:19:12 UTC
[PR] Fix event timer firing earlier than the time set. [beam]
shunping opened a new pull request, #35089:
URL: https://github.com/apache/beam/pull/35089
(no comment)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "lostluck (via GitHub)" <gi...@apache.org>.
lostluck commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2116231965
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
I mean, we have popped the arbitrary timer `e` off the heap. We see that it's time hasn't come.
Then we break out of the loop.
`e` is now lost.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "lostluck (via GitHub)" <gi...@apache.org>.
lostluck commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2116206570
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
And for completeness, SDK's don't decide what to process or not. It's the runner's job to determine what elements are ready for processing.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "lostluck (via GitHub)" <gi...@apache.org>.
lostluck commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2115113270
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
So this is to delay a prematurely potential timer? Interesting!
(I'll need to look at this off of my phone to confirm)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad merged PR #35089:
URL: https://github.com/apache/beam/pull/35089
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2115134295
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
Yes. From what I see, after the timer is set on the SDK side, it becomes one of the pending elements in the heap. If the input data is not arriving fast enough (watermark is growing slowly), at some point, the timer element is the only element in the heap. Even though the watermark is smaller than the timer firing timestamp, the runner still packs the timer element to the bundle and sends it over to the sdk.
Something I need to confirm with you is that the SDK seems to immediately run the timer callback as it sees the timer element. Is that right?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2116221911
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
> Looking again, as written this is causing data loss: You're losing the value that was popped from the heap on line 1637.
>
> The idea feels sound though (only processing what's beneath the current input watermark.)
No, there won't be a data loss there. Because if the heap has some data elements earlier than the timer element, the if statement at line 1632 guarantees that we will only put all the data elements before the timer into one bundle. As a result, the timer element will always ends up at the beginning of a bundle.
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
> Looking again, as written this is causing data loss: You're losing the value that was popped from the heap on line 1637.
>
> The idea feels sound though (only processing what's beneath the current input watermark.)
No, there won't be data loss there. Because if the heap has some data elements earlier than the timer element, the if statement at line 1632 guarantees that we will only put all the data elements before the timer into one bundle. As a result, the timer element will always ends up at the beginning of a bundle.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "lostluck (via GitHub)" <gi...@apache.org>.
lostluck commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2116231965
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
I mean, we have popped the arbitrary timer `e` off the heap. We see that it's time hasn't come.
Then we break out of the loop.
e is now lost.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "lostluck (via GitHub)" <gi...@apache.org>.
lostluck commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2116204121
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
Looking again, as written this is causing data loss: You're losing the value that was popped from the heap on line 1637.
The idea feels sound though (only processing what's beneath the current input watermark.)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35089:
URL: https://github.com/apache/beam/pull/35089#issuecomment-2921170971
Assigning reviewers:
R: @jrmccluskey for label go.
Note: If you would like to opt out of this review, comment `assign to next reviewer`.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "lostluck (via GitHub)" <gi...@apache.org>.
lostluck commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2116307659
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
If the Java prism suite continues to pass, I have no objections. I think you're right about the premature firing given the "set timer in the far future" experiment.
This might have slipped in when I split the eventTime bundle building method through the different kinds....
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Prism: Fix event-time timer firing earlier than the time set. [beam]
Posted by "shunping (via GitHub)" <gi...@apache.org>.
shunping commented on code in PR #35089:
URL: https://github.com/apache/beam/pull/35089#discussion_r2116277439
##########
sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go:
##########
@@ -1648,6 +1648,10 @@ keysPerBundle:
continue
}
holdsInBundle[e.holdTimestamp]++
+ if e.timestamp > watermark {
Review Comment:
I see. I have corrected that in the latest commit.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org