-
Notifications
You must be signed in to change notification settings - Fork 98
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
fix: getWatermark to return-1 if any processor returns -1 #402
Conversation
Signed-off-by: Yashash H L <yashashhl25@gmail.com>
Signed-off-by: Yashash H L <yashashhl25@gmail.com>
|
@whynowy, how do we handle this in |
pkg/watermark/fetch/edge_fetcher.go
Outdated
if t != -1 && t < epoch { | ||
if t == -1 { // watermark cannot be computed, perhaps a new processing unit was added or offset fell off the timeline | ||
epoch = t | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return -1 for watermark if we don't find an entry in the offset timeline store in any processor
The PR description doesn't match the logic here.
For example, if we have processors A B C, if processor A's watermark for inputOffset is -1, then we break the for loop and skip processors B and C.
The description above means if all A B C returns -1 then we return -1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the logic here
https://github.com/numaproj/numaflow/pull/402/files#diff-6af877f3295078204591f48a2180bc855394b50997f764275b25ef2ffff678daL110-R116
numaflow/pkg/watermark/fetch/edge_fetcher.go
Lines 114 to 116 in 6863142
if epoch == math.MaxInt64 { | |
epoch = -1 | |
} |
to return -1 if no entry found in any offset timeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we need to set the overall watermark to -1 if watermark cannot be fetched for any processing unit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the description
👌
do we still need this logic then?
numaflow/pkg/watermark/fetch/edge_fetcher.go
Lines 114 to 116 in 6863142
if epoch == math.MaxInt64 { | |
epoch = -1 | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need it, if there are no processors, we will return int max
Signed-off-by: Yashash H L <yashashhl25@gmail.com>
Signed-off-by: Yashash H L <yashashhl25@gmail.com>
Signed-off-by: Yashash H L <yashashhl25@gmail.com>
Signed-off-by: Yashash H L <yashashhl25@gmail.com>
Signed-off-by: Yashash H L yashashhl25@gmail.com
while fetching watermark return
-1
, if the offset timeline store returns-1
for any processor.