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 Event::modify + reflector::store helpers #907

Merged
merged 11 commits into from
May 12, 2022
Merged

Conversation

clux
Copy link
Member

@clux clux commented May 12, 2022

orginal idea - scrapped

An extension written into `Writer` to take a mutator (defaults to the identity)

The first part is the most substantial, and it required tweaking the Default + Debug implementations (which were done via Derivative before). There might be some ways to get around this with less trait restrictions on F but it always complained without this, and it seems to work well in a real word setting in example with a straight closure.

The use case for the mutator is memory improvements. Currently you can customize structs with partially typed resources, but you cannot drop the significant contributions from ObjectMeta via its managed fields. Additionally, it feels a bit wrong to me to recommend rewriting structs when what we really want is just to drop what parts of it we store, and that can be done with a mutator. By the time we use it we have already extracted the key parts for the keys in the state so it should be pretty user-safe as well.

clux added 5 commits May 12, 2022 09:27
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022

Codecov Report

Merging #907 (ee617be) into master (d0bf02f) will decrease coverage by 0.11%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##           master     #907      +/-   ##
==========================================
- Coverage   70.56%   70.44%   -0.12%     
==========================================
  Files          64       64              
  Lines        4331     4341      +10     
==========================================
+ Hits         3056     3058       +2     
- Misses       1275     1283       +8     
Impacted Files Coverage Δ
kube-runtime/src/watcher.rs 50.64% <0.00%> (-5.07%) ⬇️
kube-runtime/src/reflector/mod.rs 100.00% <100.00%> (ø)
kube-runtime/src/reflector/store.rs 95.16% <100.00%> (+0.24%) ⬆️
kube-runtime/src/wait.rs 68.00% <0.00%> (-2.00%) ⬇️

@nightkr
Copy link
Member

nightkr commented May 12, 2022

Hm.. personally I'd be more inclined to put this into the reflector, or to have this be a map step in between the watcher and reflector. It feels pretty weird to have the Store mutate things on its own when it's presented as a dumb hashmap-style data structure. Boxing the callback also becomes an optimization barrier for the compiler, and requires a virtual call.

Something along the lines of..

watcher(...)
  .map_ok(|event| event.map_objects(|mut obj| {
    obj.metadata.managed_fields = None;
    obj
  }))
  .reflector(...)

Now that we have WatchStreamExt we could even define a WatchStreamExt::map_event_objects() or something.

On the other hand, 👍 on reflector::store().

@clux
Copy link
Member Author

clux commented May 12, 2022

Yeah, that makes perfect sense. I will get back to this and try something with CustomResourceExt later if i have time. Might put this on hold before 0.72.0.

Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented May 12, 2022

Actually not too bad to get working. This is only the Event::map_event though, that's actually fine for now. Names are still a bit awkward, but gotta step away for a bit now.

Signed-off-by: clux <sszynrae@gmail.com>
/// pod.status = None;
/// });
/// ```
pub fn map_event(mut self, f: impl Fn(&mut K)) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

mutate_objects, perhaps? map implies that it operates on a fn(T) -> T rather than fn(&mut T) -> ()

Copy link
Member

Choose a reason for hiding this comment

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

I believe this could also be FnMut rather than Fn since we apply them sequentially (unless you plan on introducing parallelism later?)

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIKT the benefit to having it be FnMut is that it can capture mutable variables from a closure?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Or rather, it can capture closure variables mutably. You could still capture mutable variables immutably since they just get downgraded to immutable during the borrow.

kube-runtime/src/watcher.rs Outdated Show resolved Hide resolved
clux added 2 commits May 12, 2022 10:31
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux changed the title Allow store::Writer to configure its cache with a mutator Add Event::modify + reflector::store helpers May 12, 2022
@clux clux added the changelog-add changelog added category for prs label May 12, 2022
@clux clux added this to the 0.72.0 milestone May 12, 2022
remove generally pointless configmap_reflector
very similar to secret_reflector and we already have 4 basic ones.

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux marked this pull request as ready for review May 12, 2022 18:46
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux merged commit 7715cab into master May 12, 2022
@clux clux deleted the reflector-cache-fn branch May 12, 2022 19:02
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.

3 participants