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

feat: add configurable validation strategy by topic #745

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eliax1996
Copy link
Contributor

This pr enable the user to choose the naming strategy (and his validation) by running a POST towards /topic/<topic:str>/name_strategy/<strategy:str> endpoint and get the current used one by doing a GET towards /topic/<topic:str>/name_strategy

@eliax1996 eliax1996 force-pushed the eliax1996/enable-users-to-switch-subject-strategy-by-topic branch 4 times, most recently from a314cac to 5541f81 Compare October 30, 2023 12:18
@eliax1996 eliax1996 force-pushed the eliax1996/enable-users-to-switch-subject-strategy-by-topic branch from 5541f81 to fcd6710 Compare October 30, 2023 14:03
@eliax1996 eliax1996 marked this pull request as ready for review October 30, 2023 14:04
@eliax1996 eliax1996 requested review from a team as code owners October 30, 2023 14:04
@eliax1996 eliax1996 force-pushed the eliax1996/enable-users-to-switch-subject-strategy-by-topic branch from bce8a6e to 50e1c25 Compare October 30, 2023 16:57
@eliax1996 eliax1996 force-pushed the eliax1996/enable-users-to-switch-subject-strategy-by-topic branch from 50e1c25 to a31471c Compare October 30, 2023 16:58
Copy link
Contributor

@giuseppelillo giuseppelillo left a comment

Choose a reason for hiding this comment

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

Partial review

@@ -269,12 +282,14 @@ def validate_config(config: Config) -> None:
f"Invalid master election strategy: {master_election_strategy}, valid values are {valid_strategies}"
) from None

name_strategy = config["name_strategy"]
deafault_name_strategy = config["default_name_strategy"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

Suggested change
deafault_name_strategy = config["default_name_strategy"]
default_name_strategy = config["default_name_strategy"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah you are right, I've copy-pasted the same in the following pr

* - ``name_strategy``
- ``topic_name``
- Name strategy to use when storing schemas from the kafka rest proxy service
* - ``default_name_strategy``
Copy link
Contributor

Choose a reason for hiding this comment

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

This change would require a major version upgrade

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, I've decided to get rid of this change. This pr needs to be rebased on top of the #754. Probably I will close this pr since the design of making stateful the request for publish using the rest endpoint isn't a great idea.
It's probably mark this pr as draft

Comment on lines +235 to +238
if topic_name not in self.topic_validation_strategies:
return None

return self.topic_validation_strategies[topic_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

How about

self.topic_validation_strategies.get(topic_name)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer an exception instead of a None as return type since I'm guarding the access on the if before, am I wrong?

@@ -39,7 +45,7 @@
import logging
import time

RECORD_KEYS = ["key", "value", "partition"]
SUBJECT_VALID_POSTFIX = [SubjectType.key, SubjectType.value]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we getting rid of partition here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, if you look previously there was a tricky side effect of using the zip function.
If you run zip on two lists and the first one it's shorter than the second you will get as output a list of tuple2 with the same length of the first list. And we were exactly in that specific case 😄

@eliax1996 eliax1996 marked this pull request as draft November 7, 2023 15:39
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