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

List children exactly once for ChildSetReconciler #494

Merged
merged 1 commit into from Mar 11, 2024

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Mar 8, 2024

Previously, the ChildSetReconciler would list children once for the set and again for each child being reconciled. At best this was inefficient, at worst it introduced subtile issues when the content of the listing changed over the course of the reconcile.

Now the content of the list from the ChildSetReconciler is reused by each child that is reconciled.

Resolves #481

Previously, the ChildSetReconciler would list children once for the
set and again for each child being reconciled. At best this was
inefficient, at worst it introduced subtile issues when the content of
the listing changed over the course of the reconcile.

Now the content of the list from the ChildSetReconciler is reused by
each child that is reconciled.

Signed-off-by: Scott Andrews <scott@andrews.me>
@scothis
Copy link
Contributor Author

scothis commented Mar 8, 2024

cc @squeedee

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 71.87500% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 61.13%. Comparing base (e9224d1) to head (e90766a).

Files Patch % Lines
testing/client.go 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
- Coverage   61.18%   61.13%   -0.06%     
==========================================
  Files          28       28              
  Lines        2566     2583      +17     
==========================================
+ Hits         1570     1579       +9     
- Misses        909      917       +8     
  Partials       87       87              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@squeedee
Copy link
Collaborator

I really like the calledAtMostTimes solution for the test.

@squeedee
Copy link
Collaborator

I went back to the state of my project that caused me to raise this issue, passing 2/10 times, then pointed at this branch and reran, got 10/10 passes, so I have a lot of confidence this resolves the issue. Thanks!

@mamachanko mamachanko merged commit 7342347 into vmware-labs:main Mar 11, 2024
4 checks passed
@scothis scothis deleted the dedup-childset-lists branch March 11, 2024 16:46
@vmwclabot
Copy link

@scothis, VMware has approved your signed contributor license agreement.

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.

Determinism of IdentifyChild is confusing
4 participants