KAFKA-18026: KIP-1112, clean up graph node grace period resolution#18342
Conversation
ableegoldman
force-pushed
the
KIP-1112-cleanup-GracePeriodGraphNode
branch
from
cf90a40 to
6a6f32c
Compare
| } | ||
|
|
||
| @SuppressWarnings("rawtypes") | ||
| private static Long extractGracePeriod(final GraphNode node) { |
There was a problem hiding this comment.
Main point of this PR is to get rid of this ugly thing
There was a problem hiding this comment.
Still a good progress that we replaced four nested instanceof with just one :) Just kidding.
There was a problem hiding this comment.
haha yeah it's a step in the right direction...but it's still only just one step 😛
agavra
left a comment
agavra
left a comment
There was a problem hiding this comment.
LGTM! Thanks @ableegoldman - I always appreciate improving encapsulation and type safety 🫡
| * Represents a stateful {@link ProcessorGraphNode} where a semantic grace period is defined for the processor | ||
| * and its state. | ||
| */ | ||
| public class GracePeriodGraphNode<K, V> extends ProcessorGraphNode<K, V> { |
There was a problem hiding this comment.
I wonder if it makes sense to add something directly to ProcessorGraphNode such as ProcessorGraphNode#getWindowInformation that returns an Optional<WindowInformation> that contains grace period.
That removes the instanceof GracePeriodGraphNode check, at the cost of adding something empty to all the other nodes. I'm ambivalent as either approach works for me.
There was a problem hiding this comment.
yeah I considered that too. In the end I opted for the smaller change, as there are some other things that work similarly which would be nice to clean up, so I was thinking it might make sense to wait until everything is done and then we can consider the full picture
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
LGTM. Only nit comments.
| } | ||
|
|
||
| <KR, VR> KTable<KR, VR> build(final String aggFunctionName, | ||
| final String storeName, |
There was a problem hiding this comment.
nit: misaligned indentation?
| } | ||
|
|
||
| @SuppressWarnings("rawtypes") | ||
| private static Long extractGracePeriod(final GraphNode node) { |
There was a problem hiding this comment.
Still a good progress that we replaced four nested instanceof with just one :) Just kidding.
…pache#18342) Minor followup to apache#18195 that I split out into a separate PR since that one was getting a bit long. Should be rebased & reviewed after that one is merged. Introduces a new class for windowed graph nodes with a grace period defined to improve (slightly) the type safety Reviewers: Guozhang Wang <guozhang.wang.us@gmail.com>, Almog Gavra <almog@responsive.dev>
…pache#18342) Minor followup to apache#18195 that I split out into a separate PR since that one was getting a bit long. Should be rebased & reviewed after that one is merged. Introduces a new class for windowed graph nodes with a grace period defined to improve (slightly) the type safety Reviewers: Guozhang Wang <guozhang.wang.us@gmail.com>, Almog Gavra <almog@responsive.dev>
…pache#18342) Minor followup to apache#18195 that I split out into a separate PR since that one was getting a bit long. Should be rebased & reviewed after that one is merged. Introduces a new class for windowed graph nodes with a grace period defined to improve (slightly) the type safety Reviewers: Guozhang Wang <guozhang.wang.us@gmail.com>, Almog Gavra <almog@responsive.dev>
3 participants
Minor followup to #18195 that I split out into a separate PR since that one was getting a bit long. Should be rebased & reviewed after that one is merged.
Introduces a new class for windowed graph nodes with a grace period defined to improve (slightly) the type safety