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

#1569 | Changed default value of schema.repository.deleteSchemaPathSuffix to empty string. #1754

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

adriansobolewski
Copy link
Contributor

@adriansobolewski adriansobolewski commented Oct 18, 2023

Why on earth did I change so many lines just to change the default config value?

I've noticed that there is a "hacky" constructor with default values in SchemaRegistryRawSchemaClient. So to change the default value of deleteSchemaPathSuffix I would have to make changes in two places:

  1. In the properties file
  2. In the default constructor

After analysis, I've found out that the default constructor is used to provide schema client to hermes-frontend and hermes-consumers. Those default clients didn't even use the delete/create/validate methods, so I've introduced an read-only abstraction to get rid of this "hacky" constructor with default values.

After this change changing the deleteSchemaPathSuffix can be done just in one place.

Additionally, the code is cleaner and the read-only schema client is simpler and uses less dependencies.

If you don't like this change I can drop the commit with the refactor.

…aPathSuffix to empty string. This has been done to make requests to default Confluent schema registry match its delete url.
…des methods to read and modify the schema. Modified RawSchemaClient to provide only read methods.
@adriansobolewski adriansobolewski force-pushed the change-deleteSchemaPathSuffix-default-value branch 3 times, most recently from c2548ff to 2cf0f3f Compare October 18, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants