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

fix(outputs/azure_data_explorer): Added azureBlob controls to lower RAM usage #10179

Merged
merged 21 commits into from
Feb 1, 2022

Conversation

AsafMah
Copy link
Contributor

@AsafMah AsafMah commented Nov 25, 2021

Required for all PRs:

  • Updated associated README.md. (Internal change, doesn't need to update README)
  • Wrote appropriate unit tests. (No new functionality)
  • Pull request title or commits are in conventional commit format

(This PR is waiting until we release the next version of azure-kusto-go with this API)

(What are the guidelines if the issue wasn't raised on github? should I create it myself?)
resolves #

Added azureBlob controls to lower RAM usage

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Nov 25, 2021
@srebhan
Copy link
Member

srebhan commented Dec 21, 2021

@AsafMah can you please mark the PR as draft for the time being to prevent me to read this PR description over and over because I forgot it's not yet for review... :-D

@AsafMah AsafMah marked this pull request as draft December 21, 2021 10:20
@AsafMah AsafMah marked this pull request as ready for review January 23, 2022 08:22
@AsafMah
Copy link
Contributor Author

AsafMah commented Jan 23, 2022

@srebhan The pr is ready now

The tests seem to fail on unrelated parts - telegraf/plugins/outputs/azure_monitor
This is not my fault, right?

@powersj
Copy link
Contributor

powersj commented Jan 24, 2022

The tests seem to fail on unrelated parts - telegraf/plugins/outputs/azure_monitor This is not my fault, right?

You have updated a large number of azure libraries and I believe that is causing the tests to fail. Our nightly tests are not failing. You will need to ensure these tests continue to pass.

@srebhan
Copy link
Member

srebhan commented Jan 24, 2022

@AsafMah as @powersj stated, the tests before your PR are all green and after your PR those are not green anymore. So it might be that library updates cause the issue, but anyhow, you need to identify the source of this problem as failing tests indicate some change in behavior of the plugin. Please take a look and check what fails and why!

@AsafMah
Copy link
Contributor Author

AsafMah commented Jan 26, 2022

Ok yes, it did seem to be a problem with an upgraded version of a dependency messing with a test that tries to have mock MSI login.
Thanks to @tomconte we've solved it, so I think it's now ready to merge.

@powersj @evanphx it seems that the windows build failed because of an infrastructure problem, and I don't have the permission to re-run it, so can you do that? thanks

@powersj
Copy link
Contributor

powersj commented Jan 26, 2022

Looks like CircleCI is having issues right now. I've hit cancel, but it seems to be spinning there. I can try to check in again a bit later if someone does not get to it first.

@AsafMah
Copy link
Contributor Author

AsafMah commented Jan 27, 2022

It seems to pass now, can someone review?

@minwal
Copy link
Contributor

minwal commented Jan 28, 2022

@srebhan - All green now, can you please review?

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@AsafMah thanks for your effort. The code looks good to me, just one small style suggestion. However, it's ok for me to be merged like this.

@srebhan srebhan self-assigned this Jan 28, 2022
@srebhan srebhan added area/azure Azure plugins including eventhub_consumer, azure_storage_queue, azure_monitor plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Jan 28, 2022
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@powersj
Copy link
Contributor

powersj commented Jan 28, 2022

@AsafMah can you please run make fmt and push once more? That should get tests to pass. Thanks!

@AsafMah
Copy link
Contributor Author

AsafMah commented Jan 30, 2022

Done.

@AsafMah AsafMah requested a review from srebhan February 1, 2022 06:02
@powersj powersj merged commit b60b8d3 into influxdata:master Feb 1, 2022
powersj pushed a commit that referenced this pull request Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/azure Azure plugins including eventhub_consumer, azure_storage_queue, azure_monitor fix pr to fix corresponding bug plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants