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.

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

Hubs ADX ingestion out of memory fix #1216

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Hubs ADX ingestion out of memory fix #1216

wants to merge 2 commits into from

Conversation

flanakin
Copy link
Collaborator

🛠️ Description

Improve performance and memory consumption of the Costs_transform_v1_0() KQL function

Fixes #1188

📋 Checklist

🔬 How did you test this change?

  • 🤏 Lint tests
  • 🤞 PS -WhatIf / az validate
  • 👍 Manually deployed + verified
  • 💪 Unit tests
  • 🙌 Integration tests

🙋‍♀️ Do any of the following that apply?

  • 🚨 This is a breaking change.
  • 🤏 The change is less than 20 lines of code.

📑 Did you update docs/changelog.md?

  • ✅ Updated changelog (required for dev PRs)
  • ➡️ Will add log in a future PR (feature branch PRs only)
  • ❎ Log not needed (small/internal change)

📖 Did you update documentation?

  • ✅ Public docs in docs (required for dev)
  • ✅ Internal dev docs in src (required for dev)
  • ➡️ Will add docs in a future PR (feature branch PRs only)
  • ❎ Docs not needed (small/internal change)

@flanakin flanakin added this to the 2024-12 - December milestone Dec 16, 2024
@flanakin flanakin requested a review from MSBrett as a code owner December 16, 2024 00:17
@flanakin flanakin requested review from Copilot and MSBrett and removed request for MSBrett December 16, 2024 00:17
@microsoft-github-policy-service microsoft-github-policy-service bot added Micro PR 🔬 Very small PR that should be especially easy for newcomers Needs: Review 👀 PR that is ready to be reviewed labels Dec 16, 2024
Copilot
Copilot AI reviewed Dec 16, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
@flanakin flanakin requested a review from bwatts64 December 16, 2024 01:55
@flanakin flanakin added the Tool: FinOps hubs Data pipeline solution label Dec 16, 2024
@RolandKrummenacher
Copy link
Collaborator

I was able to reproduce the out of memory exception in my test environment:

image

I've tried to ingest a single snappy.parquet file:

image

image

Cluster size is Dev(No SLA)_Standard_D11_v2

@RolandKrummenacher
Copy link
Collaborator

By applying the fix in this PR, I was able to ingest the file successfully:

image

RolandKrummenacher
RolandKrummenacher approved these changes Dec 20, 2024
View reviewed changes
Copy link
Collaborator

@RolandKrummenacher RolandKrummenacher left a comment

Choose a reason for hiding this comment

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

See comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot Copilot left review comments

@RolandKrummenacher RolandKrummenacher RolandKrummenacher approved these changes

@MSBrett MSBrett Awaiting requested review from MSBrett MSBrett is a code owner

@arthurclares arthurclares Awaiting requested review from arthurclares

@bwatts64 bwatts64 Awaiting requested review from bwatts64

Assignees

@arthurclares arthurclares

Labels
Micro PR 🔬 Very small PR that should be especially easy for newcomers Needs: Review 👀 PR that is ready to be reviewed Tool: FinOps hubs Data pipeline solution
Projects
None yet
Milestone
2024-12 - December
Development

Successfully merging this pull request may close these issues.

Cost_Final table update policy fails with out of memory exception.
3 participants
@flanakin @RolandKrummenacher @arthurclares