Skip to content

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

Open
JPaja wants to merge 1 commit into
kube-rs:mainfrom
JPaja:feature/store_filter
Open

feat: Add filter function to reflector::Store#1681
JPaja wants to merge 1 commit into
kube-rs:mainfrom
JPaja:feature/store_filter

Conversation

@JPaja

@JPaja JPaja commented Jan 25, 2025 •
edited
Loading

Copy link
Copy Markdown

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
Terminal Add filter function to reflector::Store
031bd3e 
Signed-off-by: JPaja <jovan.pavlovic99@gmail.com>
@JPaja JPaja force-pushed the feature/store_filter branch from 7a8d546 to 031bd3e Compare January 25, 2025 19:42
@clux

clux commented Jan 27, 2025

Copy link
Copy Markdown
Member

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?

@codecov

codecov Bot commented Jan 27, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.0%. Comparing base (6a980c6) to head (031bd3e).
⚠️ Report is 378 commits behind head on main.

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%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JPaja

JPaja commented Jan 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JPaja @clux