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

Support 'enabled' configurations for offline stores #545

Merged
merged 7 commits into from
Aug 24, 2022

Conversation

enya-yx
Copy link
Collaborator

@enya-yx enya-yx commented Aug 2, 2022

For offline stores, S3/ADLS/WASB/JDBC/Snowflake, if any of its 'enabled' config variables are set to 'false' or undefined their other related environment variables will not be visited.

@xiaoyongzhu xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Aug 2, 2022
@xiaoyongzhu
Copy link
Member

Quick comment - instead just return empty string, we should also eliminate the parameter names below:

                '--s3-config', self._get_s3_config_str(),
                '--adls-config', self._get_adls_config_str(),
                '--blob-config', self._get_blob_config_str(),
                '--sql-config', self._get_sql_config_str(),
                '--snowflake-config', self._get_snowflake_config_str()

i.e. if s3 is not configured, it doesn't make sense to have --s3-config in the param list

@enya-yx
Copy link
Collaborator Author

enya-yx commented Aug 22, 2022

Quick comment - instead just return empty string, we should also eliminate the parameter names below:

                '--s3-config', self._get_s3_config_str(),
                '--adls-config', self._get_adls_config_str(),
                '--blob-config', self._get_blob_config_str(),
                '--sql-config', self._get_sql_config_str(),
                '--snowflake-config', self._get_snowflake_config_str()

i.e. if s3 is not configured, it doesn't make sense to have --s3-config in the param list

Modified the solution in new commit.

@jainr jainr self-requested a review August 23, 2022 22:25
jainr
jainr previously approved these changes Aug 23, 2022
Copy link
Collaborator

@jainr jainr left a comment

Choose a reason for hiding this comment

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

Thanks for the update, changes look good to me.

@blrchen blrchen merged commit 31414fe into feathr-ai:main Aug 24, 2022
ahlag pushed a commit to ahlag/feathr that referenced this pull request Aug 26, 2022
For offline stores, S3/ADLS/WASB/JDBC/Snowflake, if any of its 'enabled' config variables are set to 'false' or undefined their other related environment variables will not be visited.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants