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

WIP: Fix logic for regular expressions in topic_filter #944

Closed

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Dec 18, 2021

Fixing logic for filtering messages in topic_filter to respect record_options.all, regex and topic list.

Current logic for filtering messages in TopicFilter doesn't respect record_options.all in case if defined topic list at the same time.
If topic name not in topic list but match with regex it's still will be filtered out. i.e. regex doesn't work if topic list not empty and topic name not in topic list.
Also if record_options.all equal false and topic_name doesn't match with other filters TopicFilter will not filter out such topics.

Expected behavior :

  1. record_options.alll takes precedence over regex and topic list
  2. topic list shouldn't have precedence over regex. They should be addition to each other.
  3. TopicFilter::take_topic(..) should return false, if record_options.alll == false and topic name doesn't match with any other filter conditions.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@emersonknapp
Copy link
Collaborator

record_options.alll takes precedence over regex

I don't think this rule makes sense to me. If I specify --all --regex foo - should I really expect that the regex argument is invalidated? Should all only mix with exclude?

If that's the case, then I think we should have a large warning right at the beginning, if the user specifies all with either topic_list or regex, something like:

WARNING:  You provided a topic list or regex, but --all option was specified, which overrides both of those. Topic list and `--regex` will not be used.

@MichaelOrlov
Copy link
Contributor Author

I don't think this rule makes sense to me. If I specify --all --regex foo - should I really expect that the regex argument is invalidated? Should all only mix with exclude?

I think the rule makes a lot of sense to be consistent in logic. I can envision a real case when command line could be patched by some script adding --all parameter to override all other filters.
I agree about idea adding a warning if --all overrides other filters specified in settings or command line. I will add proposed warning.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:50
@MichaelOrlov
Copy link
Contributor Author

@MichaelOrlov MichaelOrlov deleted the morlov/fix-logic-for-regex-in-topic_filter branch July 13, 2024 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants