Skip to content

MINOR: reuse pseudo-topic in FKJoin#8296

Merged
vvcephei merged 4 commits intoapache:trunkfrom
vvcephei:minor-fix-pseudo-topic
Mar 14, 2020
Merged

MINOR: reuse pseudo-topic in FKJoin#8296
vvcephei merged 4 commits intoapache:trunkfrom
vvcephei:minor-fix-pseudo-topic

Conversation

@vvcephei
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei commented Mar 13, 2020

Reuse the same pseudo-topic for serializing the LHS value in the foreign-key join resolver as
we originally used to serialize it before sending the subscription request.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Mar 13, 2020

Hey @abbccdda , can you take a look at this minor patch? It's probably not a big deal, but we should use the same pseudo-topic for the same purpose.

vvcephei
vvcephei commented Mar 13, 2020
View reviewed changes
Comment thread ...afka/streams/kstream/internals/foreignkeyjoin/SubscriptionResolverJoinProcessorSupplier.java
//While we can use the source topic from where the events came from, we shouldn't serialize against it
//as it causes problems with the confluent schema registry, which requires each topic have only a single
//registered schema.
final String dummySerializationTopic = context().topic() + "-join-resolver";
Copy link
Copy Markdown
Contributor Author

@vvcephei vvcephei Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just missed this last time I made a pass and cleaned up all the pseudo-topics. We should have passed in the common one for hashing LHS values, which is declared in KTableImpl (which is what we're doing now).

abbccdda
abbccdda reviewed Mar 13, 2020
View reviewed changes
Copy link
Copy Markdown

@abbccdda abbccdda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a unit test for this change?

abbccdda
abbccdda reviewed Mar 13, 2020
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KTableImpl.java
final SubscriptionResolverJoinProcessorSupplier<K, V, VO, VR> resolverProcessorSupplier = new SubscriptionResolverJoinProcessorSupplier<>(
primaryKeyValueGetter,
valSerde == null ? null : valSerde.serializer(),
valueHashSerdePseudoTopic,
Copy link
Copy Markdown

@abbccdda abbccdda Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move the initialization of valueHashSerdePseudoTopic closer to where it is firstly used?

Copy link
Copy Markdown
Contributor Author

@vvcephei vvcephei Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thought is reasonable, but I put all the pseudo-topic definitions together so that we could see what they all are at a glance.

Copy link
Copy Markdown
Contributor Author

@vvcephei vvcephei Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus, the first use is the very first processor in the FK join, so it couldn't move down by much anyway :)

John Roesler added 2 commits March 13, 2020 17:15
@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Mar 13, 2020

Thanks for the review, @abbccdda ! Good thinking.. I've added the test.

@vvcephei vvcephei merged commit 7945cbc into apache:trunk Mar 14, 2020
@vvcephei vvcephei deleted the minor-fix-pseudo-topic branch March 14, 2020 04:04
ijuma added a commit to confluentinc/kafka that referenced this pull request Mar 17, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into ccs-master
6f7084a 
* apache-github/trunk: (39 commits)
  MINOR: cleanup and add tests to StateDirectoryTest (apache#8304)
  HOTFIX: StateDirectoryTest should use Set instead of List (apache#8305)
  MINOR: Fix build and JavaDoc warnings (apache#8291)
  MINOR: Fix kafka.server.RequestQuotaTest missing new ApiKeys. (apache#8302)
  KAFKA-9712: Catch and handle exception thrown by reflections scanner (apache#8289)
  KAFKA-9670; Reduce allocations in Metadata Response preparation (apache#8236)
  MINOR: fix Scala 2.13 build error introduced in apache#8083 (apache#8301)
  MINOR: enforce non-negative invariant for checkpointed offsets (apache#8297)
  MINOR: comment apikey types in generated switch (apache#8201)
  MINOR: Fix typo in CreateTopicsResponse.json (apache#8300)
  KIP-546: Implement describeClientQuotas and alterClientQuotas. (apache#8083)
  KAFKA-6647: Do note delete the lock file while holding the lock (apache#8267)
  KAFKA-9677: Fix consumer fetch with small consume bandwidth quotas (apache#8290)
  KAFKA-9533: Fix JavaDocs of KStream.transformValues (apache#8298)
  MINOR: reuse pseudo-topic in FKJoin (apache#8296)
  KAFKA-6145: Pt 2. Include offset sums in subscription (apache#8246)
  KAFKA-9714; Eliminate unused reference to IBP in `TransactionStateManager` (apache#8293)
  KAFKA-9718; Don't log passwords for AlterConfigs in request logs (apache#8294)
  KAFKA-8768: DeleteRecords request/response automated protocol (apache#7957)
  KAFKA-9685: Solve Set concatenation perf issue in AclAuthorizer
  ...
vvcephei added a commit that referenced this pull request Apr 29, 2020
@vvcephei
MINOR: reuse pseudo-topic in FKJoin (#8296)
e3a8223 
Reuse the same pseudo-topic for serializing the LHS value in the foreign-key join resolver as
we originally used to serialize it before sending the subscription request.

Reviewers: Boyang Chen <boyang@confluent.io>
vvcephei added a commit that referenced this pull request Apr 29, 2020
@vvcephei
MINOR: reuse pseudo-topic in FKJoin (#8296)
6314ca6 
Reuse the same pseudo-topic for serializing the LHS value in the foreign-key join resolver as
we originally used to serialize it before sending the subscription request.

Reviewers: Boyang Chen <boyang@confluent.io>
qq619618919 pushed a commit to qq619618919/kafka that referenced this pull request May 12, 2020
@vvcephei @qq619618919
MINOR: reuse pseudo-topic in FKJoin (apache#8296)
8a77760 
Reuse the same pseudo-topic for serializing the LHS value in the foreign-key join resolver as
we originally used to serialize it before sending the subscription request.

Reviewers: Boyang Chen <boyang@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abbccdda abbccdda abbccdda left review comments

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vvcephei @abbccdda