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

SAMZA-1542 refactor config classes in samza-core java to hide non-necessary interfaces #398

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fredji97
Copy link

@fredji97 fredji97 commented Jan 3, 2018

With previous inheritance, it exposed everything from Config class, which is a bad practice and is error prone. We are refactoring it to use composition over inheritance make it more robust.

Copy link
Contributor

@sborya sborya left a comment

Choose a reason for hiding this comment

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

I think it is ok. But we need to be careful with methods like getFilteredConfig(). We need to make sure (probably add it as a release note) that no module is using configs not prefixed correctly for this module.

@vjagadish1989
Copy link
Contributor

vjagadish1989 commented Jan 17, 2018

Hey @fredji97 ,
Thanks for this PR. While I'm +1 to your idea, I'm trying to ensure that the open-source codebase doesn't remain in a state where some Config classes follow inheritance while others follow composition for a long period of time. Ideally, I would love to make all changes in one-shot throughout the Samza code-base. It need not have to be a single PR but would be nice if you have all necessary PRs ready and if they can be merged in quick succession. cc: @prateekm @nickpan47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants