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

Use concurrent dictionary in related extensions calculation #5951

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Aug 5, 2024

Bug

Related: NuGet/Home#12728

Description

It's failing the insertion: dotnet/sdk#42458.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests Functional tests exist. The SDK side that caught this test out graph build scenarios, which are more challenging to replicate. Since this is blocking an insertion, I have created a follow issue. Replicate NET SDK static graph build tests  Home#13678
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@nkolev92 nkolev92 requested a review from a team as a code owner August 5, 2024 20:09
@nkolev92 nkolev92 enabled auto-merge (squash) August 5, 2024 20:09
@nkolev92 nkolev92 changed the title Use concurrent dictionary in related extensions calculation again Use concurrent dictionary in related extensions calculation Aug 5, 2024
Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Should we try to incorporate the check that failed in the SDK, or perhaps trigger the SDK tests for certain changes in our repo? That might help us catch these before insertion time.

@zivkan
Copy link
Member

zivkan commented Aug 5, 2024

  • Added tests Tests already exist

The existing tests didn't catch the bug, so from the point of view of the checklist, I don't think the existing tests count.

@nkolev92
Copy link
Member Author

nkolev92 commented Aug 5, 2024

The tests there are a bit involved and involve static graph builds, not regular builds (unrelated from static graph restore).
I'm not sure how to replicate those tests in our repo.

I can create an issue as a follow-up since this is blocking an insertion, but I'm not sure the effort is worth given that it was caught in the SDK repo.

@nkolev92
Copy link
Member Author

nkolev92 commented Aug 5, 2024

Created an issue: NuGet/Home#13678

@nkolev92 nkolev92 merged commit c6b57e4 into dev Aug 6, 2024
28 checks passed
@nkolev92 nkolev92 deleted the dev-nkolev92-useCD branch August 6, 2024 10:32
@nkolev92
Copy link
Member Author

nkolev92 commented Aug 6, 2024

Interesting thing; https://dev.azure.com/dnceng-public/public/_build/results?buildId=766638&view=ms.vss-test-web.build-test-results-tab&runId=19539484&resultId=100859&paneView=debug.

That PR had a failure due to the concurrent dictionary.

I'll look into it, maybe there's hope for an easier test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants