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

feat: Add filter function to reflector::Store #1681

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JPaja
Copy link

@JPaja JPaja commented Jan 25, 2025

Motivation

To get list of items that match predicate from store requires using 'state' function first.
This clones all elements instead of just filtered ones.

store.state().into_iter().filter(predicate).collect::<Vec<_>>()

Solution

Add function called 'filter' in store that first filters elements and than clones.

store.filter(predicate)

@JPaja JPaja force-pushed the feature/store_filter branch from 7a8d546 to 031bd3e Compare January 25, 2025 19:42
@clux
Copy link
Member

clux commented Jan 27, 2025

Hey there, thanks for this.

I think the rational to avoid cloning here makes sense, but it brings up a point we have talked about before; that maybe we should expose an iterator state() equivalent instead? We have previously received PRs wanting to extend Store and it feels like we end up re-implementing all the map accessors.

See e.g. #1634 (comment) WDYT? Is an Iterator return type something that would be suitable here?

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.0%. Comparing base (6a980c6) to head (031bd3e).

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1681     +/-   ##
=======================================
+ Coverage   75.9%   76.0%   +0.1%     
=======================================
  Files         84      84             
  Lines       7611    7640     +29     
=======================================
+ Hits        5771    5800     +29     
  Misses      1840    1840             
Files with missing lines Coverage Δ
kube-runtime/src/reflector/store.rs 97.5% <100.0%> (+0.6%) ⬆️

@JPaja
Copy link
Author

JPaja commented Jan 27, 2025

hi @clux, Yeah any solution that does not require cloning everything would work, Regarding iterator solution I'm just worried that RwLock read reference could cause problems in terms of locking out potential writes, because users are responsible of sanely dropping it.

Maybe implementing Read/AsyncRead on Cache would solve issue here, allowing AsyncReadExt to handle all possible functions

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.

2 participants