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

[pkg/stanza] Overhaul reader management #27823

Merged

Conversation

djaglowski
Copy link
Member

@djaglowski djaglowski commented Oct 18, 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.

@djaglowski djaglowski added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 18, 2023
@djaglowski
Copy link
Member Author

@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 djaglowski force-pushed the pkg-stanza-singleton-metadatas branch 2 times, most recently from 817cdd4 to e415be8 Compare October 20, 2023 01:18
@djaglowski djaglowski changed the title [pkg/stanza] Overhaul how we manage readers [pkg/stanza] Overhaul reader management Oct 20, 2023
@djaglowski djaglowski force-pushed the pkg-stanza-singleton-metadatas branch 4 times, most recently from 01f939b to d6db185 Compare October 23, 2023 19:04
@djaglowski djaglowski marked this pull request as ready for review October 23, 2023 20:14
@djaglowski djaglowski requested review from a team and jpkrohling October 23, 2023 20:14
@djaglowski djaglowski force-pushed the pkg-stanza-singleton-metadatas branch from d6db185 to 8d613f1 Compare October 23, 2023 20:26
@djaglowski djaglowski marked this pull request as draft October 23, 2023 20:30
@djaglowski
Copy link
Member Author

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.

@djaglowski djaglowski force-pushed the pkg-stanza-singleton-metadatas branch 2 times, most recently from 33e8475 to dbf5c87 Compare October 23, 2023 21:15
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 djaglowski force-pushed the pkg-stanza-singleton-metadatas branch from 6f361cd to e4df8ba Compare October 24, 2023 02:40
@djaglowski djaglowski force-pushed the pkg-stanza-singleton-metadatas branch 2 times, most recently from 98c8f32 to a4fe142 Compare October 24, 2023 16:52
@djaglowski djaglowski marked this pull request as ready for review October 24, 2023 17:32
@djaglowski
Copy link
Member Author

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 djaglowski force-pushed the pkg-stanza-singleton-metadatas branch from dd70dc5 to 72f421f Compare October 25, 2023 14:33
@djaglowski
Copy link
Member Author

This is ready for a proper review now.

@VihasMakwana
Copy link
Contributor

I've opened #27823 to extract one additional small subset of changes from this PR.

Would this be one of #28491 or #28493? Because #27823 refers to the current PR.

Copy link
Contributor

@VihasMakwana VihasMakwana left a comment

Choose a reason for hiding this comment

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

Looks good. This will surely help our problem with the asynchronous solution.

@djaglowski
Copy link
Member Author

I've opened #27823 to extract one additional small subset of changes from this PR.

Would this be one of #28491 or #28493? Because #27823 refers to the current PR.

Oops, I meant #28580. Either way, already merged. Thanks for the review.

Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

The changes look decent, however, are the changes covered by existing test cases?

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.
@djaglowski
Copy link
Member Author

Thanks for the review @MovieStoreGuy.

are the changes covered by existing test cases?

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.

@djaglowski djaglowski merged commit 599c3b1 into open-telemetry:main Oct 27, 2023
83 checks passed
@djaglowski djaglowski deleted the pkg-stanza-singleton-metadatas branch October 27, 2023 11:26
@github-actions github-actions bot added this to the next release milestone Oct 27, 2023
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
Labels
pkg/stanza Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants