KAFKA-9925: decorate pseudo-topics with app id by vvcephei · Pull Request #8574 · apache/kafka

vvcephei · 2020-04-28T16:26:46Z

ensure that pseudo-topics get correctly prefixed with the app id at run time
update test to expect the app id prefix

Committer Checklist (excluded from commit message)

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

vvcephei · 2020-04-28T16:27:07Z

+        // the decoration can't be performed until we have the configuration available when the app runs,
+        // so we pass Suppliers into the components, which they can call at run time


Hopefully, this explains what's going on here.

I understood

vvcephei · 2020-04-28T16:43:27Z

+            if (primaryKeySerializationPseudoTopic == null) {
+                primaryKeySerializationPseudoTopic = primaryKeySerializationPseudoTopicSupplier.get();
+            }


This (and below) is a bit awkward.

Our requirement is not to call the supplier until after the app starts, but we can call it any time after the app starts.

The natural place would be in configure, but unfortunately, that method is basically useless for our internal serdes. The reason is that we previously decided that configure should be called externally to the DSL, but our internal serdes are constructed internal to the DSL. Plus, configure must be called at run time (when the config is available), but by run time, we can no longer tell whether our serde is "internal" or not. So, there's no good place where we can call configure for our internal serdes.

I'm side-stepping the problem here by just invoking the supplier when we first need to use it, which is also at run time.

vvcephei · 2020-04-28T16:45:41Z

        return decoratedTopics;
    }

+    public String decoratePseudoTopic(final String topic) {


I'm adding a new public method for our specific use case here, to document that we should only need to invoke this method publicly for "pseudo" topics.

vvcephei · 2020-04-28T16:46:29Z

            // expected pseudo-topics
-            "KTABLE-FK-JOIN-SUBSCRIPTION-REGISTRATION-0000000006-topic-fk--key",
-            "KTABLE-FK-JOIN-SUBSCRIPTION-REGISTRATION-0000000006-topic-pk--key",
-            "KTABLE-FK-JOIN-SUBSCRIPTION-REGISTRATION-0000000006-topic-vh--value",
+            applicationId + "-KTABLE-FK-JOIN-SUBSCRIPTION-REGISTRATION-0000000006-topic-fk--key",
+            applicationId + "-KTABLE-FK-JOIN-SUBSCRIPTION-REGISTRATION-0000000006-topic-pk--key",
+            applicationId + "-KTABLE-FK-JOIN-SUBSCRIPTION-REGISTRATION-0000000006-topic-vh--value",


This verifies the fix: the pseudo topics should also be prefixed. I should have noticed before that they weren't.

guozhangwang · 2020-04-28T18:43:13Z

cc @abbccdda @mjsax to take a look?

arkins

Many thanks for the quick turn around for the fix, with application id appended at the front, the issue should be resolved.

vvcephei · 2020-04-29T03:30:39Z

Thanks, @arkins ! Shame is a powerful motivator :)

abbccdda

LGTM

abbccdda · 2020-04-29T20:05:05Z

+        // the decoration can't be performed until we have the configuration available when the app runs,
+        // so we pass Suppliers into the components, which they can call at run time


I understood

vvcephei · 2020-04-29T21:14:33Z

Thanks, all! I'll go ahead and merge this.

Reviewers: Boyang Chen <boyang@confluent.io>, Kin Siu

vvcephei · 2020-04-29T22:21:40Z

cherry-picked to 2.5

Reviewers: Boyang Chen <boyang@confluent.io>, Kin Siu

vvcephei · 2020-04-29T22:31:07Z

cherry-picked to 2.4

…/master` * apache-github/trunk: (45 commits) MINOR: Fix broken JMX link in docs by adding missing starting double quote (apache#8587) KAFKA-9652: Fix throttle metric in RequestChannel and request log due to KIP-219 (apache#8567) KAFKA-9922: Update demo instructions in examples README (apache#8559) KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses (apache#8442) KAFKA-9875: Make integration tests more resilient (apache#8578) KAFKA-9932: Don't load configs from ZK when the log has already been loaded (apache#8582) KAFKA-9925: decorate pseudo-topics with app id (apache#8574) KAFKA-9832: fix attempt to commit non-running tasks (apache#8580) KAFKA-9127: don't create StreamThreads for global-only topology (apache#8540) MINOR: add support for kafka 2.4 and 2.5 to downgrade test KAFKA-9176: Retry on getting local stores from KafkaStreams (apache#8568) KAFKA-9823: Follow-up, check state for handling commit error response (apache#8548) KAFKA-6145: KIP-441: Add TaskAssignor class config (apache#8541) MINOR: Fix partition numbering from 0 to P-1 instead of P in docs (apache#8572) KAFKA-9921: disable caching on stores configured to retain duplicates (apache#8564) Minor: remove redundant check in auto preferred leader election (apache#8566) MINOR: Update the link to the Raft paper in docs (apache#8560) MINOR: Fix typos in config properties in MM2 test (apache#8561) MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982) MINOR: document how to escape json parameters to ducktape tests (apache#8546) ...

There was a minor conflict in gradle.properties because the default Scala version changed upstream to Scala 2.13. I kept the upstream change. Related to this, I have updated Jenkinsfile to compile and validate with Scala 2.12 in a separate stage so that we ensure we maintain compatibility. Unlike Apache Kafka, we only run the tests with the default Scala version, which is now 2.13. * apache-github/trunk: (45 commits) MINOR: Fix broken JMX link in docs by adding missing starting double quote (apache#8587) KAFKA-9652: Fix throttle metric in RequestChannel and request log due to KIP-219 (apache#8567) KAFKA-9922: Update demo instructions in examples README (apache#8559) KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses (apache#8442) KAFKA-9875: Make integration tests more resilient (apache#8578) KAFKA-9932: Don't load configs from ZK when the log has already been loaded (apache#8582) KAFKA-9925: decorate pseudo-topics with app id (apache#8574) KAFKA-9832: fix attempt to commit non-running tasks (apache#8580) KAFKA-9127: don't create StreamThreads for global-only topology (apache#8540) MINOR: add support for kafka 2.4 and 2.5 to downgrade test KAFKA-9176: Retry on getting local stores from KafkaStreams (apache#8568) KAFKA-9823: Follow-up, check state for handling commit error response (apache#8548) KAFKA-6145: KIP-441: Add TaskAssignor class config (apache#8541) MINOR: Fix partition numbering from 0 to P-1 instead of P in docs (apache#8572) KAFKA-9921: disable caching on stores configured to retain duplicates (apache#8564) Minor: remove redundant check in auto preferred leader election (apache#8566) MINOR: Update the link to the Raft paper in docs (apache#8560) MINOR: Fix typos in config properties in MM2 test (apache#8561) MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982) MINOR: document how to escape json parameters to ducktape tests (apache#8546) ...

Reviewers: Boyang Chen <boyang@confluent.io>, Kin Siu

KAFKA-9925: decorate pseudo-topics with app id

8e8467d

vvcephei commented Apr 28, 2020

View reviewed changes

John Roesler added 2 commits April 28, 2020 11:51

checkstyle

a863182

checkstyle

ea21699

guozhangwang requested a review from mjsax April 28, 2020 18:43

arkins approved these changes Apr 28, 2020

View reviewed changes

abbccdda approved these changes Apr 29, 2020

View reviewed changes

vvcephei merged commit 688f2e9 into apache:trunk Apr 29, 2020

vvcephei deleted the kafka-9925-decorate-pseudo-topics branch April 29, 2020 21:17

vvcephei added a commit that referenced this pull request Apr 29, 2020

KAFKA-9925: decorate pseudo-topics with app id (#8574)

d1ae75d

Reviewers: Boyang Chen <boyang@confluent.io>, Kin Siu

vvcephei added a commit that referenced this pull request Apr 29, 2020

KAFKA-9925: decorate pseudo-topics with app id (#8574)

306c008

Reviewers: Boyang Chen <boyang@confluent.io>, Kin Siu

qq619618919 pushed a commit to qq619618919/kafka that referenced this pull request May 12, 2020

KAFKA-9925: decorate pseudo-topics with app id (apache#8574)

bc04077

Reviewers: Boyang Chen <boyang@confluent.io>, Kin Siu

		// the decoration can't be performed until we have the configuration available when the app runs,
		// so we pass Suppliers into the components, which they can call at run time

Conversation

vvcephei commented Apr 28, 2020

Committer Checklist (excluded from commit message)

Uh oh!

vvcephei Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

abbccdda Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

vvcephei Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

vvcephei Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

vvcephei Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Apr 28, 2020

Uh oh!

arkins left a comment

Choose a reason for hiding this comment

Uh oh!

vvcephei commented Apr 29, 2020

Uh oh!

abbccdda left a comment

Choose a reason for hiding this comment

Uh oh!

abbccdda Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

vvcephei commented Apr 29, 2020

Uh oh!

vvcephei commented Apr 29, 2020

Uh oh!

vvcephei commented Apr 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants