Repository navigation
Allow sources to render arbitrary dataflows - #18055
Conversation
16ffb6d to
778ac52
Compare
525915c to
66434fc
Compare
44fc0a7 to
e1c6f0a
Compare
|
@philip-stoev this PR changes quite a few things in case you want to direct your attention to it |
e1c6f0a to
714008a
Compare
guswynn
left a comment
guswynn
left a comment
There was a problem hiding this comment.
Amazing! This is looking so much better! I just have a few comments and questions! Also, I feel like keeping the OffsetCommitter trait, but its your call!
I fear that this trait will have the same shortcomings as the original data traits in that you can't describe very well your topology. For example if to commit offsets there is some information that needs to be exchanged between the workers then you're hosed so I think it's much easier for each source to handle the stream as it pleases. I'm not super happy with the |
guswynn
left a comment
guswynn
left a comment
There was a problem hiding this comment.
I'm not super happy with the offset_commit_loop thingies in this PR but I think they can be written in a better way. I didn't want to make this PR too big for this reason
this seems like a reasonable path forward
aljoscha
left a comment
aljoscha
left a comment
There was a problem hiding this comment.
This is looking pretty sweet! I had some comments about operator naming and logging, but those should be easy to address.
What's up with the failing nighly, though?
It's a type, so it should be there. Signed-off-by: Petros Angelatos <[email protected]>
I ran the |
ed54848 to
69c4b67
Compare
69c4b67 to
579c71f
Compare
This commit defines the `SourceRender` trait which supercedes all the previous ways a source could be defined. Sources are now able to render arbitrary dataflows, taking advantange of multiple communicating timely operators when needed. A lot of the auxiliary types that used to support the old traits have been removed. Signed-off-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
This commit applies a few simplifications in the process of converting the source to the `SourceRender` trait by being smarter with how capabilities are managed. Specifically, the operator does *not* downgrade its capability for every message. Instead, it exhausts all the available messages from the partitions which are emitted with the same capability but at different times (a la DD triplets) and then downgrades the capability of each partition onces. This produces a significant performance boost in the feature benchmarks: ``` NAME | THIS | OTHER | Regression? | 'THIS' is: ---------------------------------------------------------------------------------------------------- KafkaEnvelopeNoneBytes | 11.524 | 12.409 | no | 7.1 pct faster KafkaUpsert | 3.163 | 5.172 | no | 38.8 pct faster KafkaUpsertUnique | 4.436 | 5.180 | no | 14.4 pct faster ``` Signed-off-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
d31c063 to
2f0e05c
Compare
Instead, log the error to the source status collection and request a `SuspendAndRestart`. This commit also introduces a mandatory 30s backoff before a `SuspendAndResume` takes place, to limit hot retry loops. This comes at the expense of latency in the case of a transient error; unfortunately implementing a smarter retry policy (like exponential backoff) is difficult. Fix #18491.
2f0e05c to
f171a23
Compare
Motivation
This PR defines the
SourceRendertrait which supersedes all the previous ways a source could be defined. Sources are now able to render arbitrary dataflows, taking advantage of multiple communicating timely operators when needed.A lot of the auxiliary types that used to support the old traits have been removed.
Of special mention is the conversion of the Kafka source implementation to the new trait which applies a few simplifications by being smarter with how capabilities are managed.
Specifically, the operator does not downgrade its capability for every message. Instead, it exhausts all the available messages from the partitions which are emitted with the same capability but at different times (a la DD triplets) and then downgrades the capability of each partition once.
This produces a significant performance boost in the feature benchmarks:
Tips for reviewer
Reviewing commit by commit is recommended. Also, definitely read the new doc comments before reading the code to understand how the new trait works.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.