[adapters] Ignore clock for pipeline completion when now_http_driven.#6452
Conversation
When a pipeline uses NOW, then normally it cannot ever signal that the pipeline is completed, because the results of the pipeline can always change as time goes on. However, when the user has configured the clock to change only as driven by explicit requests, this is no longer the case, so we then can ignore the clock for the purpose of pipeline_complete. Related: #5076 Related: #4340 Signed-off-by: Ben Pfaff <blp@feldera.com>
blp
added
connectors
| // | ||
| // We also don't require the clock connector to be at end of input | ||
| // if it's configured to advance only when the user sends a request. | ||
| let ignore = name.contains(".api-ingress-") |
There was a problem hiding this comment.
Couldn't this be a trait method on the endpoint?
There was a problem hiding this comment.
Yes, it could and arguably should. I think I'll leave that refactoring for the next time we have to touch this code.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
mythical-fred
left a comment
mythical-fred
left a comment
There was a problem hiding this comment.
The logic is sound — when the user drives the clock via HTTP, the now endpoint should not block pipeline completion. The refactoring from a single all() chain to an explicit loop with comments is more readable too.
Missing automated test coverage though. pipeline_complete() now has a new conditional branch that could regress silently. A unit test that constructs a ControllerStatus with a non-EOI "now" endpoint and now_http_driven = true and asserts pipeline_complete() == true would nail this down.
| // | ||
| // We also don't require the clock connector to be at end of input | ||
| // if it's configured to advance only when the user sends a request. | ||
| let ignore = name.contains(".api-ingress-") |
There was a problem hiding this comment.
The string "now" is a magic value identifying the clock endpoint. If this name is defined elsewhere as a constant, it would be safer to reference that constant here. If it isn't, consider extracting it as one — a rename of the clock endpoint would silently break this check.
3 participants
When a pipeline uses NOW, then normally it cannot ever signal that the pipeline is completed, because the results of the pipeline can always change as time goes on. However, when the user has configured the clock to change only as driven by explicit requests, this is no longer the case, so we then can ignore the clock for the purpose of pipeline_complete.
Related: #5076
Related: #4340
Describe Manual Test Plan
I tested this with a workload script.