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-2406: Add ConfigLoader interface and default PropertiesConfigLo… #1226

Closed
wants to merge 6 commits into from
Closed

Conversation

kw2542
Copy link
Contributor

@kw2542 kw2542 commented Nov 27, 2019

Add ConfigLoader interface and default PropertiesConfigLoader implementation, which will be used on AM to fetch configs.

The existing ConfigFactory interface will be deprecated and removed as we will not fetch configs on start up script run-app.sh anymore.

Design:
https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner

Changes:

  1. Add ConfigLoader interface which will be used in job coordinator to retrieve config from.
  2. Add PropertiesConfigLoader which is a default implementation of ConfigLoader that reads a local properties file given the file path.

API Changes:
N/A. This is part of a series PRs, detailed information will be provided in the last/main PR.
Upgrade Instructions:
N/A. This is part of a series PRs, detailed information will be provided in the last/main PR.
Usage Instructions:
N/A. This is part of a series PRs, detailed information will be provided in the last/main PR.

Tests:

  1. Add TestPropertiesConfigLoader to unit test PropertiesConfigLoader.

…ader implementation

Design:
  https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner

Changes:
  1. Add ConfigLoader interface which will be used in job coordinator to retrieve config from.
  2. Add PropertiesConfigLoader which is a default implementation of ConfigLoader that reads a local properties file given the file path.

Tests:
  1. Add TestPropertiesConfigLoader to unit test PropertiesConfigLoader.
@prateekm
Copy link
Contributor

prateekm commented Dec 3, 2019

@bharathkk and @abhishekshivanna offered to look at this.

@kw2542 Can you update usage/upgrade instructions in the description with changes required for using this new feature? This information will be collected for the release notes and changelog for the next release.

Let's also fix the commit title (wrapping).

/**
* Build a {@link org.apache.samza.config.Config}
*/
public interface ConfigLoader {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add more descriptive java docs?

  1. It will be helpful if you can talk about how and when this interface is used.
  2. What if ConfigLoader implementors want to handle dependency injection? Do we need to provide a factory for the config loader implementors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added more descriptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a default constructor with config is too restrictive and violates separation of concerns, if we were to evolve ConfigLoader. ConfigLoader should not have to deal with dependency object construction and use. I'd prefer a factory that manages dependency injection for ConfigLoader so that its evolution doesn't affect the framework as long as the contract is maintained.

@mynameborat
Copy link
Contributor

@bharathkk and @abhishekshivanna offered to look at this.

@kw2542 Can you update usage/upgrade instructions in the description with changes required for using this new feature? This information will be collected for the release notes and changelog for the next release.

Let's also fix the commit title (wrapping).

@kw2542 Can you also update the API changes in the description and add more details about this API (internal vs external) etc

@abhishekshivanna
Copy link
Contributor

@kw2542 , could you also clarify as part of this patch the reason we need this new implementation over the existing config-factory (eg: PropertiesConfigFactory) and config-path implementation ? Is this meant to be a replacement for the way we do config loading today ? If so, should we deprecate the old way ?

@kw2542
Copy link
Contributor Author

kw2542 commented Dec 10, 2019

@kw2542 , could you also clarify as part of this patch the reason we need this new implementation over the existing config-factory (eg: PropertiesConfigFactory) and config-path implementation ? Is this meant to be a replacement for the way we do config loading today ? If so, should we deprecate the old way ?

@kw2542 kw2542 closed this Dec 10, 2019
@kw2542 kw2542 reopened this Dec 10, 2019
@kw2542
Copy link
Contributor Author

kw2542 commented Dec 10, 2019

@kw2542 , could you also clarify as part of this patch the reason we need this new implementation over the existing config-factory (eg: PropertiesConfigFactory) and config-path implementation ? Is this meant to be a replacement for the way we do config loading today ? If so, should we deprecate the old way ?

Updated both the PR description as well as SEP itself.

ConfigLoader will replace ConfigFactory interface, which will be deleted in the following PRs.

@kw2542
Copy link
Contributor Author

kw2542 commented Dec 17, 2019

Discard to start a new PR clean.

@kw2542 kw2542 closed this Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants