Kafka-14748: Relax non-null FK left-join requirement#14107
Kafka-14748: Relax non-null FK left-join requirement#14107wcarlson5 merged 8 commits intoapache:trunkfrom
Conversation
florin-akermann
force-pushed
the
KAFKA-14748
branch
3 times, most recently
from
19dfb4b to
fdf411c
Compare
florin-akermann
force-pushed
the
KAFKA-14748
branch
3 times, most recently
from
e51c5cc to
47a84fc
Compare
florin-akermann
force-pushed
the
KAFKA-14748
branch
from
47a84fc to
3d1ed37
Compare
|
I'm going to start my review of this today. Hopefully can be actionable sometime tomorrow. |
wcarlson5
left a comment
wcarlson5
left a comment
There was a problem hiding this comment.
Just a couple of comments
| </code> | ||
| </pre> | ||
| </p> | ||
|
|
There was a problem hiding this comment.
Thanks for remembering the docs update!
|
|
||
| private void forward(final Record<K, Change<V>> record, final KO foreignKey, final Instruction deleteKeyNoPropagate) { | ||
| final SubscriptionWrapper<K> wrapper = new SubscriptionWrapper<>( | ||
| hash(record), |
There was a problem hiding this comment.
How many times is this record rehased?
There was a problem hiding this comment.
there are branches where it is hashed twice. I puhsed a change to cache the hash value.
| instruction = PROPAGATE_ONLY_IF_FK_VAL_AVAILABLE; | ||
| } | ||
| final KO newForeignKey = foreignKeyExtractor.apply(record.value().newValue); | ||
| final KO newForeignKey = record.value().newValue == null ? null : foreignKeyExtractor.apply(record.value().newValue); |
There was a problem hiding this comment.
this seems redundant with line 165. That check the newValue is not null
There was a problem hiding this comment.
indeed!, thanks, removed.
…changing foreign key
florin-akermann
force-pushed
the
KAFKA-14748
branch
from
8138f80 to
4f77165
Compare
Thank you @wcarlson5 for the comments. While I have your attention could we revisit the adjutments done to the optimization 'drop null key records on repartitioning' as part of #14174 (comment). I was wondering whether it would make sense to adjust the optimization from 'filter null-key records in repartition nodes if no left or outer-join is downstream' to 'filter null-key records in repartition nodes if no left or outer-join node is part of this branch of the topology (aka no left or outer-join is up- or downstream of this repartition node)' |
|
I'm not sure, either option for the optimization is fine with me as long as it's well documented. I'm good with how the PR is for now. I'm going to merge it to get it in before feature freeze for 3.7 |
Relax non-null FK left-join requirement. Testing Strategy: Inject extractor which returns null on first or second element. Reviewers: Walker Carlson <wcarlson@apace.org>
Relax non-null FK left-join requirement. Testing Strategy: Inject extractor which returns null on first or second element. Reviewers: Walker Carlson <wcarlson@apace.org>
Relax non-null FK left-join requirement. Testing Strategy: Inject extractor which returns null on first or second element. Reviewers: Walker Carlson <wcarlson@apace.org>
Relax non-null FK left-join requirement. Testing Strategy: Inject extractor which returns null on first or second element. Reviewers: Walker Carlson <wcarlson@apace.org>
| if (oldForeignKey != null && !Arrays.equals(serialize(newForeignKey), serialize(oldForeignKey))) { | ||
| forward(record, oldForeignKey, DELETE_KEY_AND_PROPAGATE); | ||
| } | ||
| forward(record, newForeignKey, PROPAGATE_NULL_IF_NO_FK_VAL_AVAILABLE); |
There was a problem hiding this comment.
@florin-akermann @wcarlson5 -- Seems we introduces a bug here.
There was a problem hiding this comment.
@mjsax omg, thanks for the flag! Looks like @AyoubOm is addressing it in https://issues.apache.org/jira/browse/KAFKA-16394 already? Else i'll adress it.
3 participants
Relax non-null FK left-join requirement.
Testing Strategy: Inject extractor which returns null on first or second element.
Committer Checklist (excluded from commit message)