Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Support 'enabled' configurations for offline stores#545

Merged
blrchen merged 7 commits into
feathr-ai:mainfeathr-ai/feathr:mainfrom
enya-yx:enya/enable-flagsenya-yx/feathr:enya/enable-flagsCopy head branch name to clipboard
Aug 24, 2022
Merged

Support 'enabled' configurations for offline stores#545
blrchen merged 7 commits into
feathr-ai:mainfeathr-ai/feathr:mainfrom
enya-yx:enya/enable-flagsenya-yx/feathr:enya/enable-flagsCopy head branch name to clipboard

Conversation

@enya-yx
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.