KAFKA-10316: Consider renaming getter method for Interactive Queries #9120

johnthotekat · 2020-08-04T07:32:07Z

*Deprecated the existing getters and added new getters activeHost(), standbyHosts(), and partition() in KeyQueryMetadata.java.

*Updated the existing Tests to use the new getters of KeyQueryMetadata.java . Below are the Test classes that were updated with new getters.

OptimizedKTableIntegrationTest.java
QueryableStateIntegrationTest.java
StoreQueryIntegrationTest.java
StreamsMetadataStateTest.java

*Corrected JavaDoc nits in the deprecated methods.

@mjsax @abbccdda @vvcephei @brary Since you've voted on the KIP, please feel free to review.

Committer Checklist (excluded from commit message)

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

* Deprecated the existing getters and added new getters `activeHost()`, `standbyHosts()`, and `partition()` to KeyQueryMetadata.java. * Update the existing Tests to use the new getters of KeyQueryMetadata.java . Below are the Test classes that were updated with new getters. OptimizedKTableIntegrationTest.java QueryableStateIntegrationTest.java StoreQueryIntegrationTest.java StreamsMetadataStateTest.java

mjsax

Thanks for the PR @johnthotekat!

Just some JavaDocs nits. I understand it's c&p but it seem the original JavaDocs are not "ideal" and we should just improve it -- if you want, you could also update the JavaDocs of the deprecated methods accordingly (I leave it up to you if you want to do this or not -- I am fine either way).

mjsax · 2020-08-04T14:46:28Z

streams/src/main/java/org/apache/kafka/streams/KeyQueryMetadata.java

    public int getPartition() {
        return partition;
    }

+    /**
+     * Get the Active streams instance for given key


Nit: [a]ctive (i understand that it's just c&p but it seem wrong in the original JavaDoc, too.

streams -> Kafka Streams

Also, missing . at the end of the sentence.

I'm with you, we should fix it, for the deprecated ones too.

Fixed.

mjsax · 2020-08-04T14:47:16Z

streams/src/main/java/org/apache/kafka/streams/KeyQueryMetadata.java

+    }
+
+    /**
+     * Get the Streams instances that host the key as standbys


nit: Streams -> Kafka Streams

Also, missing . at the end of the sentence.

mjsax · 2020-08-04T14:47:47Z

streams/src/main/java/org/apache/kafka/streams/KeyQueryMetadata.java

+    }
+
+    /**
+     * Get the Store partition corresponding to the key.


mjsax · 2020-08-04T14:50:51Z

Retest this please.

* Deprecated the existing getters and added new getters `activeHost()`, `standbyHosts()`, and `partition()` to KeyQueryMetadata.java. * Update the existing Tests to use the new getters of KeyQueryMetadata.java . Below are the Test classes that were updated with new getters. OptimizedKTableIntegrationTest.java QueryableStateIntegrationTest.java StoreQueryIntegrationTest.java StreamsMetadataStateTest.java * Correcting some JavaDoc nits.

brary · 2020-08-04T16:25:10Z

streams/src/main/java/org/apache/kafka/streams/KeyQueryMetadata.java

+    /**
+     * Get the store partition corresponding to the key.
+     *
+     * @return store partition number


I feel it should be "key partition number" instead of store.

We are fetching the state store partition number corresponding to the key with this getter and hence that description I believe.
More over, this is a copy paste work, we copied existing getter's with a new name and deprecated the old getter's.
Any specific reason why you feel that the description should be "key partition number" ?

@mjsax Please correct me if I'm wrong.

I wrote the old getters & javadocs but now I feel that key partition number better conveys the meaning than store partition number.
The class KeyQueryMetadata contains the information for a key being requested via query. So, the partition number we are returning here is the partition to which the key belongs, it has no relation to any store yet. You will of course use this partition information to fetch the data corresponding to the key from any store but still that doesn't change the fact that this partition number is related to the key rather than the store.
I am fine either way though.

IMHO, "key partition number" sound like as if the key would be partitioned, and we get the partition number for the "key partition". But in fact, the store is partitioned and thus I think the current "store partition number" is ok as-is.

Sure, that's fine.

brary · 2020-08-04T16:26:38Z

streams/src/main/java/org/apache/kafka/streams/KeyQueryMetadata.java

+    }
+
+    /**
+     * Get the store partition corresponding to the key.


Get the partition number corresponding to the key.

Relatable to the comments from line 106 right ?

mjsax · 2020-08-04T23:13:44Z

Retest this please.

mjsax · 2020-08-04T23:13:59Z

Retest this please.

brary · 2020-08-05T04:28:10Z

Thanks @johnthotekat for the PR. LGTM!

Merge apache/kafka to johnthotekat/kafka

…tps://github.com/johnthotekat/kafka into KAFKA-10316-renaming-getter-method

johnthotekat · 2020-08-05T14:55:48Z

@mjsax One of the test failed for me in local, This one - KafkaAdminClientTest#testMetadataRetries.
It's unrelated to the changes in this PR.
Rest of the tests PASSED without any issues.

I guess its the same issue we are addressing with https://issues.apache.org/jira/browse/KAFKA-10311 ? There is also an open PR under this.

org.apache.kafka.clients.admin.KafkaAdminClientTest > testMetadataRetries FAILED
    java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.TimeoutException: Call(callName=describeTopics, deadlineMs=1596638067201, tries=1, nextAllowedTryMs=1596638067302) timed out at 1596638067202 after 1 attempt(s)
        at org.apache.kafka.common.internals.KafkaFutureImpl.wrapAndThrow(KafkaFutureImpl.java:45)
        at org.apache.kafka.common.internals.KafkaFutureImpl.access$000(KafkaFutureImpl.java:32)
        at org.apache.kafka.common.internals.KafkaFutureImpl$SingleWaiter.await(KafkaFutureImpl.java:89)
        at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:260)
        at org.apache.kafka.clients.admin.KafkaAdminClientTest.testMetadataRetries(KafkaAdminClientTest.java:997)

        Caused by:
        org.apache.kafka.common.errors.TimeoutException: Call(callName=describeTopics, deadlineMs=1596638067201, tries=1, nextAllowedTryMs=1596638067302) timed out at 1596638067202 after 1 attempt(s)

            Caused by:
            org.apache.kafka.common.errors.TimeoutException: Timed out waiting for a node assignment.

mjsax · 2020-08-05T21:45:56Z

@johnthotekat Some testa are know to be flaky and we try to work on them to avoid those build issues. It's constant effort to keep the build stable. Will trigger a new Jenkins run and can merge after it passed.

I also saw that you merge changes from trunk and pushed an update. As long as there a not merge conflicts, this is not necessary. It also "invalidates" the Jenkins test result and we need to rerun Jenkins. Thus, only push new stuff into the PR if necessary. Otherwise, the merging gets delayed unnecessarily. (Also note, the "Retest this please" comments are not for you -- they trigger Jenkins (if Jenkins cooperates...) to rerun and you can ignore those comments).

mjsax · 2020-08-05T21:46:04Z

Retest this please.

mjsax · 2020-08-05T21:48:24Z

Retest this please.

mjsax · 2020-08-05T21:49:24Z

Retest this please.

mjsax · 2020-08-05T21:50:20Z

Ok to test.

mjsax · 2020-08-05T21:50:46Z

I'll try again later... Jenkins seems to be on holidays...

mjsax · 2020-08-06T00:45:19Z

Retest this please.

johnthotekat · 2020-08-06T03:38:59Z

@johnthotekat Some testa are know to be flaky and we try to work on them to avoid those build issues. It's constant effort to keep the build stable. Will trigger a new Jenkins run and can merge after it passed.

I also saw that you merge changes from trunk and pushed an update. As long as there a not merge conflicts, this is not necessary. It also "invalidates" the Jenkins test result and we need to rerun Jenkins. Thus, only push new stuff into the PR if necessary. Otherwise, the merging gets delayed unnecessarily. (Also note, the "Retest this please" comments are not for you -- they trigger Jenkins (if Jenkins cooperates...) to rerun and you can ignore those comments).

I did a pull to my branch just to make sure that my branch has all the recent changes (reason behind is the flaky test and I saw the PR for that flaky fix) and that's how that merge from trunk came in. I made sure it didn't have any conflicts. I'll make a note of your point. Thanks.

And I really thought you put in the comments "Retest this please". :)

mjsax · 2020-08-06T18:16:07Z

Thanks for the KIP and PR @johnthotekat. Merged to trunk.

mjsax · 2020-08-06T18:18:43Z

One more thing: could you do a second PR to update the docs accordingly? We should list the KIP at lest in docs/streams/upgrade-guide.html (there should be already a section for 2.7 release). And maybe also check the actual documentation if we need to do any updates theres, too?

johnthotekat · 2020-08-07T05:00:17Z

@mjsax Made the changes and raised PR : #9133
Checked the actual documentations, we're good here.

Updated Streams documentation with the new method details in KeyQueryMetadata. apache#9120

mjsax added kip Requires or implements a KIP producer labels Aug 4, 2020

mjsax reviewed Aug 4, 2020

View reviewed changes

brary reviewed Aug 4, 2020

View reviewed changes

johnthotekat and others added 2 commits August 5, 2020 18:32

Merge pull request #1 from apache/trunk

8279213

Merge apache/kafka to johnthotekat/kafka

Merge branches 'KAFKA-10316-renaming-getter-method' and 'trunk' of ht…

9d0b97f

…tps://github.com/johnthotekat/kafka into KAFKA-10316-renaming-getter-method

mjsax added streams and removed producer labels Aug 5, 2020

mjsax merged commit e7316f3 into apache:trunk Aug 6, 2020

johnthotekat mentioned this pull request Aug 7, 2020

KAFKA-10316: Updated Kafka Streams upgrade notes. #9133

Closed

johnthotekat pushed a commit to johnthotekat/kafka that referenced this pull request Aug 8, 2020

KAFKA-10316- Consider renaming getter method for Interactive Queries

71fae02

Updated Streams documentation with the new method details in KeyQueryMetadata. apache#9120

johnthotekat mentioned this pull request Aug 8, 2020

KAFKA-10316 Updated Kafka Streams upgrade-guide.html #9146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-10316: Consider renaming getter method for Interactive Queries #9120

KAFKA-10316: Consider renaming getter method for Interactive Queries #9120

johnthotekat commented Aug 4, 2020 •

edited

mjsax left a comment

mjsax Aug 4, 2020

johnthotekat Aug 4, 2020

mjsax Aug 4, 2020

johnthotekat Aug 4, 2020

mjsax Aug 4, 2020

johnthotekat Aug 4, 2020

mjsax commented Aug 4, 2020

brary Aug 4, 2020

johnthotekat Aug 4, 2020 •

edited

johnthotekat Aug 4, 2020

brary Aug 4, 2020

mjsax Aug 4, 2020

brary Aug 5, 2020

brary Aug 4, 2020

johnthotekat Aug 4, 2020

mjsax commented Aug 4, 2020

mjsax commented Aug 4, 2020

brary commented Aug 5, 2020

johnthotekat commented Aug 5, 2020 •

edited

mjsax commented Aug 5, 2020 •

edited

mjsax commented Aug 5, 2020

mjsax commented Aug 5, 2020

mjsax commented Aug 5, 2020

mjsax commented Aug 5, 2020

mjsax commented Aug 5, 2020

mjsax commented Aug 6, 2020

johnthotekat commented Aug 6, 2020

mjsax commented Aug 6, 2020

mjsax commented Aug 6, 2020

johnthotekat commented Aug 7, 2020

KAFKA-10316: Consider renaming getter method for Interactive Queries #9120

KAFKA-10316: Consider renaming getter method for Interactive Queries #9120

Conversation

johnthotekat commented Aug 4, 2020 • edited

Committer Checklist (excluded from commit message)

mjsax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax commented Aug 4, 2020

Choose a reason for hiding this comment

johnthotekat Aug 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax commented Aug 4, 2020

mjsax commented Aug 4, 2020

brary commented Aug 5, 2020

johnthotekat commented Aug 5, 2020 • edited

mjsax commented Aug 5, 2020 • edited

mjsax commented Aug 5, 2020

mjsax commented Aug 5, 2020

mjsax commented Aug 5, 2020

mjsax commented Aug 5, 2020

mjsax commented Aug 5, 2020

mjsax commented Aug 6, 2020

johnthotekat commented Aug 6, 2020

mjsax commented Aug 6, 2020

mjsax commented Aug 6, 2020

johnthotekat commented Aug 7, 2020

johnthotekat commented Aug 4, 2020 •

edited

johnthotekat Aug 4, 2020 •

edited

johnthotekat commented Aug 5, 2020 •

edited

mjsax commented Aug 5, 2020 •

edited