Skip to content

KAFKA-14647: Moving TopicFilter to server-common/utils#13158

Merged
mimaison merged 9 commits intoapache:trunkfrom
vamossagar12:KAFKA-14647
Jul 18, 2023
Merged

KAFKA-14647: Moving TopicFilter to server-common/utils#13158
mimaison merged 9 commits intoapache:trunkfrom
vamossagar12:KAFKA-14647

Conversation

@vamossagar12
Copy link
Copy Markdown
Contributor

@vamossagar12 vamossagar12 commented Jan 24, 2023

Moving TopicFilter to server-common/utils

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jan 24, 2023

@fvaleri , plz review this PR.

fvaleri
fvaleri suggested changes Jan 25, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@fvaleri fvaleri left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @vamossagar12, thanks for working on this!

I left some comments, but there are build errors.

In this PR, it is probably a good idea to also move the TopicPartitionFilter and related code contained in GetOffsetShell. They are very similar and of general use.

Comment thread server-common/src/main/java/org/apache/kafka/server/util/TopicFilter.java Outdated
Comment thread server-common/src/test/java/org/apache/kafka/server/util/TopicFilterTest.java Outdated
Comment thread server-common/src/main/java/org/apache/kafka/server/util/TopicFilter.java Outdated
Comment thread server-common/src/main/java/org/apache/kafka/server/util/TopicFilter.java Outdated
Comment thread server-common/src/main/java/org/apache/kafka/server/util/IncludeList.java Outdated
Comment thread server-common/src/test/java/org/apache/kafka/server/util/TopicFilterTest.java Outdated
@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jan 26, 2023

TopicPartitionFilter

@fvaleri , sorry about the build failures. I hadn't run the checkstyle for core on my local before pushing. I will work on the TopicPartitionFilter class, rest of the comments have been addressed/answered. Thanks for reviewing.

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jan 31, 2023

Thanks @fvaleri , I made the changes.

fvaleri
fvaleri reviewed Feb 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@fvaleri fvaleri left a comment

Choose a reason for hiding this comment

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

Hi @vamossagar12, I left few comments, but looks mostly good.

I guess we also need to add some unit tests for PartitionFilter and TopicPartitionFilter, like we have for TopicFilter. They are in GetOffsetShellParsingTest, which should rather test the tool interface not filters.

Comment thread checkstyle/import-control.xml Outdated
Comment thread server-common/src/main/java/org/apache/kafka/server/util/TopicPartitionFilter.java Outdated
Comment thread server-common/src/main/java/org/apache/kafka/server/util/TopicPartitionFilter.java Outdated
Comment thread server-common/src/test/java/org/apache/kafka/server/util/TopicFilterTest.java Outdated
@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Feb 8, 2023

Thanks @fvaleri , I addressed the review comments. Regarding unit tests for PartitionFilter and TopicPartitionFilter, I checked the tests in GetOffsetShellParsingTest and they are pretty comprehensive. I also think the interface per se is being tested in GetOffsetShellTest so I think we can let the tests remain in GetOffsetShellParsingTest. LMKWYT.

@fvaleri
Copy link
Copy Markdown
Contributor

fvaleri commented Feb 8, 2023

Hi @vamossagar12 this looks good, but I still think we should move GetOffsetShellParsingTest to TopicPartitionFilterTest removing any reference to GetOffsetShell (there is already a test for that) and create a new PartitionFilterTest, which should be similar to TopicFilterTest.

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Feb 15, 2023

Hi @vamossagar12 this looks good, but I still think we should move GetOffsetShellParsingTest to TopicPartitionFilterTest removing any reference to GetOffsetShell (there is already a test for that) and create a new PartitionFilterTest, which should be similar to TopicFilterTest.

Thanks @fvaleri . I tried doing this today. A problem that I see is that GetOffsetShellParsingTest makes use of 2 methods from GetOffsetShell namely createTopicPartitionFilterWithTopicAndPartitionPattern and createTopicPartitionFilterWithPatternList. Those 2 methods are also eventually used in GetOffsetShell when creating topicPartitionFilter. Ideally these 2 methods can reside in ToolsUtils and have both tests and GetOffsetShell reference it.
I see there's also Move GetOffsetShell to tools. Can we do the move as part of that effort or here? WDYS?

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Mar 18, 2023

What is the justification for this? For something to be in server-common, it needs to be used by multiple modules.

@fvaleri
Copy link
Copy Markdown
Contributor

fvaleri commented Apr 12, 2023

@ijuma these classes are currently used in both tools and core modules. Not all tools will be migrated in the same release and we also have the deprecated MirrorMaker 1, which we do not plan to move.

@ruslankrivoshein ruslankrivoshein mentioned this pull request Apr 13, 2023
Merged
@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented May 18, 2023

Sorry I completely missed this. Thanks @fvaleri . @ijuma do you agree with the reasoning provided by Federico?

@dengziming
Copy link
Copy Markdown
Member

dengziming commented May 19, 2023 •
edited
Loading

TopicFilter and TopicPartitionFilter are very customized and simple interfaces, it's unnecessary to rewrite them currently IMO, at least currently, they are only used in a few tools, especially TopicPartitionFilter which are only used in GetOffsetShell.

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jun 5, 2023

In that case, would it make sense to close this PR @fvaleri ?

@fvaleri
Copy link
Copy Markdown
Contributor

fvaleri commented Jun 5, 2023

In that case, would it make sense to close this PR @fvaleri ?

Why? We can discuss where to place these interfaces, but I would keep the work that has been done. It's just another step towards KAFKA-14524.

@fvaleri fvaleri requested a review from ijuma June 5, 2023 15:39
@fvaleri fvaleri added the tools label Jun 5, 2023
@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jun 7, 2023 •
edited
Loading

I see. TBH I haven't followed that migration effort off-late. I think there are build failures as well which I haven't addressed yet.

@ruslankrivoshein
Copy link
Copy Markdown
Contributor

ruslankrivoshein commented Jun 12, 2023

I would keep the work that has been done

I agree. This work is already in use in another PR.

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jun 19, 2023

I would keep the work that has been done

I agree. This work is already in use in another PR.

I see. In that case, I need to fix the build issue and ask folks for reviews then. Will try to do so as soon as I can. Thanks for notifying.

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jun 30, 2023

@ruslankrivoshein , I have fixed the checkstyle issues. Also, I believe that the other comment here has been addressed by you in the PR #13562.
Sorry for the long wait here- it just went off my radar. @fvaleri , do you think this is looking fine now?

@fvaleri
Copy link
Copy Markdown
Contributor

fvaleri commented Jun 30, 2023

@vamossagar12 LGTM. Thanks.

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jun 30, 2023

Thanks @fvaleri . Hmm I see 101 test failures. 92 existing and 9 new. Atleast the new ones look unrelated..

@vamossagar12 vamossagar12 requested a review from dengziming June 30, 2023 16:24
@fvaleri fvaleri mentioned this pull request Jul 8, 2023
Merged
3 tasks
@fvaleri fvaleri requested a review from mimaison July 12, 2023 09:09
@mimaison
Copy link
Copy Markdown
Member

mimaison commented Jul 12, 2023

I took a look at the changes and they look fine. If I understand correctly we're moving these classes to server-common while we have to keep MirrorMaker. Then in Kafka 4.0 (and assuming KAFKA-14525 is complete) we will be able to move all these classes to tools. Is that right?

@fvaleri
Copy link
Copy Markdown
Contributor

fvaleri commented Jul 12, 2023 •
edited
Loading

Yes, once the MirrorMaker1 dependency will be gone in 4.0.0, we can move them to tools. I added a note in KAFKA-14705.

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jul 12, 2023

I took a look at the changes and they look fine. If I understand correctly we're moving these classes to server-common while we have to keep MirrorMaker. Then in Kafka 4.0 (and assuming KAFKA-14525 is complete) we will be able to move all these classes to tools. Is that right?

Thanks @mimaison , yes that's the idea.

@vamossagar12
Copy link
Copy Markdown
Contributor Author

vamossagar12 commented Jul 18, 2023

@mimaison can this be merged if all changes look fine?

mimaison
mimaison approved these changes Jul 18, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks!

@mimaison mimaison merged commit fa5b493 into apache:trunk Jul 18, 2023
jolshan added a commit to confluentinc/kafka that referenced this pull request Jul 18, 2023
@jolshan
Merge remote-tracking branch 'ak/trunk' into mergeFromAK
d293a57 
* ak/trunk: (110 commits)
  MINOR: Update docs to include ZK deprecation notice and information (apache#14031)
  KAFKA-15091: Fix misleading Javadoc for SourceTask::commit (apache#13948)
  KAFKA-14669: Use the generated docs for MirrorMaker configs in the doc (apache#13658)
  KAFKA-14953: Add tiered storage related metrics (apache#13944)
  KAFKA-15121: Implement the alterOffsets method in the FileStreamSourceConnector and the FileStreamSinkConnector (apache#13945)
  Revert "MINOR: Update .asf.yaml file with refreshed github_whitelist, and collaborators" (apache#14037)
  MINOR: Update .asf.yaml file with refreshed github_whitelist, and collaborators
  KAFKA-14737: Move kafka.utils.json to server-common (apache#13585)
  KAFKA-14647: Move TopicFilter to server-common/utils (apache#13158)
  MINOR: remove unused variable in examples (apache#14021)
  ...
jeqo pushed a commit to jeqo/kafka that referenced this pull request Jul 21, 2023
@vamossagar12 @jeqo
KAFKA-14647: Move TopicFilter to server-common/utils (apache#13158)
2a79266 
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Federico Valeri <fedevaleri@gmail.com>
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
@vamossagar12 @Cerchie
KAFKA-14647: Move TopicFilter to server-common/utils (apache#13158)
9ee61ce 
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Federico Valeri <fedevaleri@gmail.com>
jeqo pushed a commit to aiven/kafka that referenced this pull request Aug 15, 2023
@vamossagar12 @jeqo
KAFKA-14647: Move TopicFilter to server-common/utils (apache#13158)
699b0aa 
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Federico Valeri <fedevaleri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mimaison mimaison mimaison approved these changes

@ijuma ijuma Awaiting requested review from ijuma

@dengziming dengziming Awaiting requested review from dengziming

+1 more reviewer

@fvaleri fvaleri fvaleri requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tools

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vamossagar12 @fvaleri @ijuma @dengziming @ruslankrivoshein @mimaison