Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker] Disable EntryFilters for system topics #20514

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jun 6, 2023

Motivation

Applying EntryFilters for system topics is not very useful and could cause problems.
I noticed this while investigating the flaky AdminApi2Test.

Modifications

Skip applying entry filters for system topics.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added type/bug The PR fixed a bug or issue reported a bug area/broker ready-to-test labels Jun 6, 2023
@lhotari lhotari added this to the 3.1.0 milestone Jun 6, 2023
@lhotari lhotari requested review from eolivelli and nicoloboschi June 6, 2023 18:25
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 6, 2023
@mattisonchao
Copy link
Member

mattisonchao commented Jun 7, 2023

IMO, We should consider a new way to handle the system topic because we do a lot of similar logic to filter system topics. :(

Not only the entry filter but also some policies etc.

@lhotari lhotari marked this pull request as draft June 7, 2023 04:56
@lhotari lhotari force-pushed the lh-disable-entry-filters-for-systemtopics branch from e78de0c to c8f8100 Compare June 7, 2023 05:05
@lhotari lhotari marked this pull request as ready for review June 7, 2023 05:11
@lhotari
Copy link
Member Author

lhotari commented Jun 7, 2023

/pulsarbot rerun-failure-checks

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@michaeljmarshall
Copy link
Member

IMO, We should consider a new way to handle the system topic because we do a lot of similar logic to filter system topics. :(

Not only the entry filter but also some policies etc.

I was mentioning the same thing to @lhotari earlier today. This is one reason I have pushed for a more formal definition of system topics on the ML over the past two years.

At the moment, their design feels very accidental instead of intentional. System topics continue to get more exceptions, and we continue to add dependencies upon the system topic concept.

Maybe we should restart a discussion on this topic.

@lhotari
Copy link
Member Author

lhotari commented Jun 7, 2023

IMO, We should consider a new way to handle the system topic because we do a lot of similar logic to filter system topics. :(
Not only the entry filter but also some policies etc.

I was mentioning the same thing to @lhotari earlier today. This is one reason I have pushed for a more formal definition of system topics on the ML over the past two years.

At the moment, their design feels very accidental instead of intentional. System topics continue to get more exceptions, and we continue to add dependencies upon the system topic concept.

Maybe we should restart a discussion on this topic.

@mattisonchao @michaeljmarshall +1. Yes, it would be good to restart it.

In KOP there are topics like __transaction_state and __consumer_offsets. These don't seem to be system topics from Pulsar's perspective at the moment. That's another example of more exceptions.

@lhotari
Copy link
Member Author

lhotari commented Jun 7, 2023

For some reason this PR isn't picking up the recent changes I pushed to the branch. That happened in another PR a while ago. Let's see if I'll need to create a new PR to pick up the changes.

@lhotari lhotari closed this Jun 7, 2023
@lhotari lhotari reopened this Jun 7, 2023
@lhotari
Copy link
Member Author

lhotari commented Jun 7, 2023

oh, GitHub Status page shows that there are on-going issues: https://www.githubstatus.com/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants