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

[chore][pkg/stanza] move file handling to internal package #31506

Merged

Conversation

VihasMakwana
Copy link
Contributor

Description: Intrdocue a new internal package tracker to handle file operations.

@djaglowski
Copy link
Member

Thanks @VihasMakwana. This is on my list, just haven't found time to fully think it through yet.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this @VihasMakwana. I think overall this looks pretty good but I suggest a slightly different line for separating manager vs tracker responsibilities.

One thing I will call out specifically is that I think it's helpful to look at the notion of a tracker as something which will eventually have alternative implementations. So the api we establish here could be a prototype for a tracker interface. Then we can keep this implementation in place and use a feature gate to try out your async implementation. See #31534 for another example of what could be tracker implementation (which in that case would intentionally not remember files at all). Also see #31596 for a bigger picture explanation of where I think this is headed.

pkg/stanza/fileconsumer/internal/tracker/tracker.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/internal/tracker/tracker.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/internal/tracker/tracker.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/internal/tracker/tracker.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/internal/tracker/tracker.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/internal/tracker/tracker_other.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/internal/tracker/tracker_other.go Outdated Show resolved Hide resolved
@VihasMakwana
Copy link
Contributor Author

@djaglowski the CI's green. can you take a look?

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this. Finally getting caught up after KubeCon.

This looks really good. I just have one nit

@djaglowski djaglowski merged commit 22cc472 into open-telemetry:main Apr 3, 2024
142 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 3, 2024
ycombinator pushed a commit to ycombinator/opentelemetry-collector-contrib that referenced this pull request Apr 9, 2024
…metry#31506)

**Description:** Intrdocue a new internal package `tracker` to handle
file operations.
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this pull request May 8, 2024
…metry#31506)

**Description:** Intrdocue a new internal package `tracker` to handle
file operations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants