Skip to content

Support 'enabled' configurations for offline stores #545

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.

Sign up for GitHub

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

Jump to bottom
Merged
blrchen merged 7 commits into from
Aug 24, 2022
Merged

Support 'enabled' configurations for offline stores #545

blrchen 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

xiaoyongzhu commented Aug 2, 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

@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
View reviewed changes
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
@enya-yx @ahlag
Support 'enabled' configurations for offline stores (feathr-ai#545)
0008290 
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.
@enya-yx enya-yx mentioned this pull request Sep 21, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xiaoyongzhu xiaoyongzhu xiaoyongzhu approved these changes

@blrchen blrchen blrchen approved these changes

@jainr jainr jainr left review comments

@windoze windoze Awaiting requested review from windoze

@Yuqing-cat Yuqing-cat Awaiting requested review from Yuqing-cat

@hangfei hangfei Awaiting requested review from hangfei

Assignees

No one assigned

Labels

safe to test Tag to execute build pipeline for a PR from forked repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@enya-yx @xiaoyongzhu @blrchen @jainr