KAFKA-9487: Follow-up PR of Kafka-9445 by brary · Pull Request #8033 · apache/kafka

brary · 2020-02-02T06:56:45Z

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

guozhangwang

The non unit-testing part LGTM. @mjsax could you take a look at the unit tests and see if they have addressed all your comments?

vinothchandar

Left few comments..

vinothchandar · 2020-02-03T19:42:08Z

    public static <T> StoreQueryParams<T> fromNameAndType(final String storeName,
                                                          final QueryableStoreType<T>  queryableStoreType) {
-        return new StoreQueryParams<T>(storeName, queryableStoreType);
+        return new StoreQueryParams<T>(storeName, queryableStoreType, null, false);


Could we use a different sentinel than null for the partition field?. Does an Optional make sense?

Actually we had a long discussion on keeping this null and adding it in Java doc that null means all partitions. Do you think this is important to start a discussion on the thread again? :)

dont worry about it :) .. Happy to budge on stylistic things and leave it to the committers' call

vinothchandar · 2020-02-03T19:53:13Z

        return results;
    }

+    public List<String> sourceTopicsForStore(final String storeName) {


a side note: Should we also consider the map stateStoreNameToSourceRegex here?

I am not sure, do you think it will identify more topic groups? At the end of the day we are identifying a topic groupId to which a store belongs to.

On a side note, if we do want to add stateStoreNameToSourceRegex along with stateStoreNameToSourceTopics, then we can probably return a set instead of list in this function and iterate over both of above one by one.

brary · 2020-02-04T17:24:27Z

Left few comments..

Thanks @guozhangwang and @vinothchandar . Addressed a few comments and have replied to some left.

vvcephei

Hey, thanks for the update @brary .

IIUC, the main motivation for this PR was some follow-up comments from @mjsax . It would be good to get a +1 from him before merging. Maybe after the other comments are addressed.

brary · 2020-02-05T08:10:45Z

Hey, thanks for the update @brary .

IIUC, the main motivation for this PR was some follow-up comments from @mjsax . It would be good to get a +1 from him before merging. Maybe after the other comments are addressed.

Thanks @vvcephei for the comments. I have made the changes suggested by you as well.

@mjsax I guess everyone else has reviewed and their comments have been incorporated. It would be great if you can give one last look and if it's okay merge it.

mjsax

Thanks for the PR. Couple of minor comments. Also added one more reply on the original PR that should be addressed in this PR (https://github.com/apache/kafka/pull/7984/files#r375517935).

vvcephei

Thanks for addressing my comments!

mjsax · 2020-02-05T23:34:55Z

@vinothchandar @vvcephei I would still propose to rename the new class to StoreQueryParameters -- it's not a big deal to rename even after feature freeze IHMO.

Thoughts?

vinothchandar · 2020-02-06T05:59:17Z

StoreQueryParameters does sound better to me. but leave it you all. :)

brary · 2020-02-06T08:56:05Z

@mjsax I have changed the class to StoreQueryParameters and addressed the comment you gave in earlier PR. There were few checkstyles which were failing in ValueAndTimestampSerializerTest unrelated to this PR. I have fixed those as well.

mjsax

Thanks @brary! LGTM. I leave it to @vvcephei to merge this PR to trunk and cherry-pick to 2.5 branch.

vvcephei · 2020-02-10T02:31:13Z

Thanks @mjsax . I'll take care of it tomorrow.

I don't see any test results, so I'll ask Jenkins to re-run it:

Retest this, please.

vvcephei · 2020-02-10T18:06:53Z

Unrelated test failures:
kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdAllGroups
kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdWithMixOfSuccessAndError

Proceeding to merge.

Follows up on the original PR for KAFKA-9445 to address a final round of feedback Cherry-pick of d76fa1b from trunk Reviewers: John Roesler <vvcephei@apache.org>, Matthias J. Sax <matthias@confluent.io>

vvcephei · 2020-02-10T21:20:59Z

Cherry-picked onto 2.5 as 3e77458

Follows up on the original PR for KAFKA-9445 to address a final round of feedback Cherry-pick of d76fa1b from trunk Reviewers: John Roesler <vvcephei@apache.org>, Matthias J. Sax <matthias@confluent.io>

Addressing code review comments of 9445

c438c5d

brary mentioned this pull request Feb 2, 2020

KAFKA-9445: Allow adding changes to allow serving from a specific partition #7984

Merged

3 tasks

brary requested a review from mjsax February 2, 2020 06:57

mjsax changed the title ~~[KAFKA-9487] : Follow-up PR of Kafka-9445~~ KAFKA-9487: Follow-up PR of Kafka-9445 Feb 2, 2020

mjsax added the streams label Feb 2, 2020

mjsax requested a review from vvcephei February 2, 2020 18:21

guozhangwang reviewed Feb 3, 2020

View reviewed changes

vinothchandar reviewed Feb 3, 2020

View reviewed changes

Addressing code review comments

decbe53

vvcephei requested changes Feb 4, 2020

View reviewed changes

Addressing code review comments

7ba5423

Merge branch 'trunk' into navinderbrar-Followup-KAFKA-9445

da685d9

mjsax reviewed Feb 5, 2020

View reviewed changes

vvcephei reviewed Feb 5, 2020

View reviewed changes

brary added 2 commits February 6, 2020 12:32

Changing StoreQueryParams to StoreQueryParameters

0f115ea

Resolving merge conflicts

8801c60

mjsax approved these changes Feb 8, 2020

View reviewed changes

vvcephei merged commit d76fa1b into apache:trunk Feb 10, 2020

mjsax added the kip Requires or implements a KIP label Jun 12, 2020

Conversation

brary commented Feb 2, 2020

Committer Checklist (excluded from commit message)

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vinothchandar Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

brary Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

vinothchandar Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vinothchandar Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

brary Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

brary commented Feb 4, 2020

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brary commented Feb 5, 2020

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Feb 5, 2020

Uh oh!

vinothchandar commented Feb 6, 2020

Uh oh!

brary commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

vvcephei commented Feb 10, 2020

Uh oh!

vvcephei commented Feb 10, 2020

Uh oh!

vvcephei commented Feb 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

brary commented Feb 6, 2020 •

edited

Loading