-
Notifications
You must be signed in to change notification settings - Fork 86
feat: add configurable validation strategy by topic #745
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
base: main
Are you sure you want to change the base?
feat: add configurable validation strategy by topic #745
Conversation
a314cac to
5541f81
Compare
5541f81 to
fcd6710
Compare
bce8a6e to
50e1c25
Compare
50e1c25 to
a31471c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
| ) from None | ||
|
|
||
| name_strategy = config["name_strategy"] | ||
| deafault_name_strategy = config["default_name_strategy"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
| deafault_name_strategy = config["default_name_strategy"] | |
| default_name_strategy = config["default_name_strategy"] |
There was a problem hiding this comment.
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`` |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| if topic_name not in self.topic_validation_strategies: | ||
| return None | ||
|
|
||
| return self.topic_validation_strategies[topic_name] |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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?
| import time | ||
|
|
||
| RECORD_KEYS = ["key", "value", "partition"] | ||
| SUBJECT_VALID_POSTFIX = [SubjectType.key, SubjectType.value] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😄
This pr enable the user to choose the naming strategy (and his validation) by running a
POSTtowards/topic/<topic:str>/name_strategy/<strategy:str>endpoint and get the current used one by doing aGETtowards/topic/<topic:str>/name_strategy