chore: Generalize file server around paths provider #2541
Conversation
MOZGIII
force-pushed
the
file-server-paths-provider
branch
from
9db5557 to
9c3866a
Compare
MOZGIII
requested review from
LucioFranco and
lukesteensen
and removed request for
LucioFranco
| current_glob_iter: Option<glob::Paths>, | ||
| } | ||
|
|
||
| impl<'a> Iterator for Iter<'a> { |
There was a problem hiding this comment.
It seems like we're taking on a decent amount of additional complexity to implement Iterator instead of just building a Vec. Given that we can only expose Vec via the API, why not go for a simpler implementation?
There was a problem hiding this comment.
I've pushed an alternative implementation that's more efficient than Vec. It has some issues though: it's definitely more complex and verbose. But the good thing is it doesn't allocate the files list in memory.
I'm considering ditching that implementation and switching back to Vec, but simpler, to trade-off the code simplicity for the runtime performance.
What do you think?
There was a problem hiding this comment.
While this is a very cool implementation, I don't think this functionality is performance-sensitive enough to justify the additional complexity. I would rather start with something that minimizes our maintenance overhead and then optimize if/when this code is determined to be a bottleneck.
There was a problem hiding this comment.
Sounds good! I’m going to replace the custom iter impl with a composition of combinators.
Signed-off-by: MOZGIII <[email protected]>
…ider Signed-off-by: MOZGIII <[email protected]>
Signed-off-by: MOZGIII <[email protected]>
Signed-off-by: MOZGIII <[email protected]>
MOZGIII
force-pushed
the
file-server-paths-provider
branch
from
9c3866a to
785f6a2
Compare
Signed-off-by: MOZGIII <[email protected]>
Signed-off-by: MOZGIII <[email protected]>
MOZGIII
force-pushed
the
file-server-paths-provider
branch
from
0228c05 to
95fcb0e
Compare
Signed-off-by: MOZGIII <[email protected]>
* Add the path provider trait and implement glob using it Signed-off-by: MOZGIII <[email protected]> * Swap the glob implmenetation at the FileServer with generic PathsProvider Signed-off-by: MOZGIII <[email protected]> * Integrate updated at `file_source` crate into the file source Signed-off-by: MOZGIII <[email protected]> * Switch PathsProvider to IntoIterator Signed-off-by: MOZGIII <[email protected]> * Implement a more advanced and efficient PathProvider for Glob Signed-off-by: MOZGIII <[email protected]> * Switch back to Vec but with a simpler implementation Signed-off-by: MOZGIII <[email protected]> * Update CODEOWNERS Signed-off-by: MOZGIII <[email protected]> Signed-off-by: Brian Menges <[email protected]>
This PR generalizes the
file_source::FileServeraround the ways it obtains the lists of paths to work with.The rationale for this change is we'll be using an alternative way of obtaining the lists of paths at the k8s implementation - it'll be based on the continuously updated data obtained from the k8s API.
An alternative to this change was to implement a new solution for file reading from scratch. I discouraged that because we already have an excellent implementation that can be generalized to fit the needs.
I was cautious to limit the scope of the change to the minimum, to decrease the effect on the other components.