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

Add find_all method to Store #1634

Closed

Conversation

pando85
Copy link
Contributor

@pando85 pando85 commented Nov 10, 2024

Resolves #1633.

Motivation

Solution

@pando85 pando85 force-pushed the feat/add-find-all-method-for-stores branch from b3348dc to 9f04a48 Compare November 10, 2024 18:13
@pando85
Copy link
Contributor Author

pando85 commented Nov 10, 2024

I tried to return an Slice or an iterator but it requires a collect anyway because store is behind a RwLock.

@clux clux added the changelog-add changelog added category for prs label Nov 11, 2024
@clux clux added this to the 0.97.0 milestone Nov 11, 2024
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Thanks a lot! LGTM.

(Please disregard lint error, false positive known issue silenced in #1636 )

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 75.3%. Comparing base (9c402a6) to head (9f04a48).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kube-runtime/src/reflector/store.rs 0.0% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1634     +/-   ##
=======================================
- Coverage   75.3%   75.3%   -0.0%     
=======================================
  Files         82      82             
  Lines       7348    7351      +3     
=======================================
  Hits        5528    5528             
- Misses      1820    1823      +3     
Files with missing lines Coverage Δ
kube-runtime/src/reflector/store.rs 94.7% <0.0%> (-2.2%) ⬇️

@SOF3
Copy link
Contributor

SOF3 commented Nov 11, 2024

it requires a collect anyway because store is behind a RwLock.

can't you move the RwLockReadGuard into the returned iterator?

@pando85
Copy link
Contributor Author

pando85 commented Nov 11, 2024

I don't understand your point @SOF3 . Could you elaborate on it a bit?

@pando85 pando85 force-pushed the feat/add-find-all-method-for-stores branch from 9f04a48 to 2da179a Compare November 11, 2024 17:42
Resolves kube-rs#1633.

Signed-off-by: Alexander Gil <[email protected]>
@pando85 pando85 force-pushed the feat/add-find-all-method-for-stores branch from 2da179a to 1882a3f Compare November 13, 2024 06:17
@SOF3
Copy link
Contributor

SOF3 commented Nov 13, 2024

I am just confused why we need so many special methods on Store instead of exposing a map-like API that provides iter(). Is it simply because we don't want to implement a custom Iterator type that owns its own RwLockReadGuard?

@pando85
Copy link
Contributor Author

pando85 commented Nov 13, 2024

@SOF3 that's a good point. In deed, we already have the state method which returns an iterator through all the objects.

I can close this and use that method.

Thank you both.

@pando85 pando85 closed this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase flexibility on store public API
3 participants