[pkg/stanza] Overhaul reader management #27823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@VihasMakwana, I've mentioned previously a need to simplify the way we manage readers, which should in turn simplify the design of your proposal. I believe I finally have substantial step in the right direction. This does not immediately enable your changes but I think it is a good start towards narrowing the problem which asynchronous polling must solve. I plan to break this into smaller PRs but I'd welcome your careful review of these preliminary changes. |
djaglowski
force-pushed
the
pkg-stanza-singleton-metadatas
branch
2 times, most recently
from
817cdd4
to
e415be8
Compare
djaglowski
changed the title
djaglowski
force-pushed
the
pkg-stanza-singleton-metadatas
branch
4 times, most recently
from
01f939b
to
d6db185
Compare
djaglowski
force-pushed
the
pkg-stanza-singleton-metadatas
branch
from
d6db185
to
8d613f1
Compare
|
This PR is currently ready but I've moved it back to a draft because I think it could be made less complex but pulling a few changes into smaller PRs. |
This was referenced Oct 23, 2023
djaglowski
force-pushed
the
pkg-stanza-singleton-metadatas
branch
2 times, most recently
from
33e8475
to
dbf5c87
Compare
djaglowski
added a commit
that referenced
this pull request
Oct 24, 2023
Follows #28419 This discards the separate "roller" and implements the same functionality directly in `fileconsumer.Manager`. The motivation for this is to move towards a system of managing files where each file is managed by only one list at a time. This PR retains two overlapping slices of readers (`previousPollFiles` and `knownFiles`), but the functionality does not change. #27823 should get us the rest of the way there.
djaglowski
force-pushed
the
pkg-stanza-singleton-metadatas
branch
from
6f361cd
to
e4df8ba
Compare
djaglowski
force-pushed
the
pkg-stanza-singleton-metadatas
branch
2 times, most recently
from
98c8f32
to
a4fe142
Compare
djaglowski
force-pushed
the
pkg-stanza-singleton-metadatas
branch
from
a4fe142
to
be9ffed
Compare
|
I've opened #27823 to extract one additional small subset of changes from this PR. |
djaglowski
added a commit
that referenced
this pull request
Oct 25, 2023
Although the persister is generally expected, we can easily protect against cases where it is not provided and save some work as well. This becomes more important with #27823 which interacts with the persister during the Stop function.
djaglowski
force-pushed
the
pkg-stanza-singleton-metadatas
branch
from
dd70dc5
to
72f421f
Compare
|
This is ready for a proper review now. |
VihasMakwana
approved these changes
Oct 26, 2023
MovieStoreGuy
approved these changes
Oct 27, 2023
sigilioso
pushed a commit
to carlossscastro/opentelemetry-collector-contrib
that referenced
this pull request
Oct 27, 2023
…metry#28451) Follows open-telemetry#28419 This discards the separate "roller" and implements the same functionality directly in `fileconsumer.Manager`. The motivation for this is to move towards a system of managing files where each file is managed by only one list at a time. This PR retains two overlapping slices of readers (`previousPollFiles` and `knownFiles`), but the functionality does not change. open-telemetry#27823 should get us the rest of the way there.
sigilioso
pushed a commit
to carlossscastro/opentelemetry-collector-contrib
that referenced
this pull request
Oct 27, 2023
…28580) Although the persister is generally expected, we can easily protect against cases where it is not provided and save some work as well. This becomes more important with open-telemetry#27823 which interacts with the persister during the Stop function.
|
Thanks for the review @MovieStoreGuy.
I think we have pretty good coverage for overall functionality. However, this change is in part motivated by the need to simplify and decompose the package in order to make it more testable. |
jmsnll
pushed a commit
to jmsnll/opentelemetry-collector-contrib
that referenced
this pull request
Nov 12, 2023
…metry#28451) Follows open-telemetry#28419 This discards the separate "roller" and implements the same functionality directly in `fileconsumer.Manager`. The motivation for this is to move towards a system of managing files where each file is managed by only one list at a time. This PR retains two overlapping slices of readers (`previousPollFiles` and `knownFiles`), but the functionality does not change. open-telemetry#27823 should get us the rest of the way there.
jmsnll
pushed a commit
to jmsnll/opentelemetry-collector-contrib
that referenced
this pull request
Nov 12, 2023
…28580) Although the persister is generally expected, we can easily protect against cases where it is not provided and save some work as well. This becomes more important with open-telemetry#27823 which interacts with the persister during the Stop function.
jmsnll
pushed a commit
to jmsnll/opentelemetry-collector-contrib
that referenced
this pull request
Nov 12, 2023
- Change `knownFiles` to `[]*reader.Metadata`. No files are held here. - Introduce new `previousPollReaders` list, which holds open files from the previous poll interval. If for any reason a file is closed, it should immediately be removed from the list and its metadata should be added to `knownFiles`. - Introduce notion that each `reader.Metadata` should be treated as a singleton. When a reader is closed, it pops out its `Metadata` so it can be appended to `knownFiles`.
RoryCrispin
pushed a commit
to ClickHouse/opentelemetry-collector-contrib
that referenced
this pull request
Nov 24, 2023
…28580) Although the persister is generally expected, we can easily protect against cases where it is not provided and save some work as well. This becomes more important with open-telemetry#27823 which interacts with the persister during the Stop function.
RoryCrispin
pushed a commit
to ClickHouse/opentelemetry-collector-contrib
that referenced
this pull request
Nov 24, 2023
- Change `knownFiles` to `[]*reader.Metadata`. No files are held here. - Introduce new `previousPollReaders` list, which holds open files from the previous poll interval. If for any reason a file is closed, it should immediately be removed from the list and its metadata should be added to `knownFiles`. - Introduce notion that each `reader.Metadata` should be treated as a singleton. When a reader is closed, it pops out its `Metadata` so it can be appended to `knownFiles`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
knownFilesto[]*reader.Metadata. No files are held here.previousPollReaderslist, which holds open files from the previous poll interval. If for any reason a file is closed, it should immediately be removed from the list and its metadata should be added toknownFiles.reader.Metadatashould be treated as a singleton. When a reader is closed, it pops out itsMetadataso it can be appended toknownFiles.