-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[BUGFIX] Disable init analytics as it was not really getting entirely disabled when specified on data context config #11276
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
base: develop
Are you sure you want to change the base?
Conversation
👷 Deploy request for niobium-lead-7998 pending review.Visit the deploys page to approve it
|
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: [email protected] |
3 similar comments
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: [email protected] |
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: [email protected] |
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: [email protected] |
5a7f8fb
to
1a23fa8
Compare
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: [email protected] |
1a23fa8
to
8d04a68
Compare
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: [email protected] |
8d04a68
to
5af98c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. This looks reasonable. However, there are a few places in the code where init_analytics
is invoked. Would you be willing to wrap these other calls in a similar check? If not we can follow up with that later.
Secondly, and this is where I'm requesting changes, the tests do need a little refactoring. test_analytics_enabled_on_load
is no longer an appropriate name for the test method (it probably wasn't in the first place). Since we need to explicitly ensure that init_analytics
is not called, there should be tests to assert that it is not called when the expected_value
is False.
Let me know if you have any questions
…s is not involved. Move try except block to cover permission denied or other errors while checking file existence
thanks for the feedback @wookasz . I have renamed one test and also enhanced it (and 1 more) to capture when it should be enabled and invoke init_analytics and when not. |
It seems init_analytics is being executed even when people is disabling analytics.
Inside this execution, there seems to be a method self._get_oss_id() searching for a root location
cls._ROOT_CONF_FILE.exists()
which is producing a permission denied error when used inside forEachBatch streaming on Databricks DBR15.4.This PR proposes a fix to avoid invoking the init_analytics method if people disable it via config.
If there is other suggestion for a workaround, while this PR is not release, please let me know as this is a bit critical for us.
You can find more context below: