Skip to content

[chore][pkg/fileconsumer] - Move archive into separate package #39353

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

Merged

Conversation

VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Apr 11, 2025

Description

Move archive into a separate package to make code more testable and less complicated overall.
This also changes function instantiateTracker to instantiateTrackerAndArchive a little bit to initialize the archive and pass it to the tracker for writing older offsets.

This PR also drops some of the edge cases to reduce the complexity. I'll work on them later

Link to tracking issue

Relates #38056

@VihasMakwana VihasMakwana changed the title [chore/fileconsumer] - Move archive into separate package [chore][pkg/fileconsumer] - Move archive into separate package Apr 11, 2025
@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from fc31fc0 to 7c4a971 Compare April 11, 2025 19:17
@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from 7c4a971 to 391371d Compare April 11, 2025 19:19
@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from 6987a29 to 2c1d990 Compare April 18, 2025 19:44
@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from 2c1d990 to 937e278 Compare April 18, 2025 19:45
@VihasMakwana
Copy link
Contributor Author

@djaglowski can you take a look here? I've added tests and addressed the comments.

@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from c63da6b to ab5f217 Compare April 24, 2025 21:17
@VihasMakwana VihasMakwana requested a review from djaglowski April 24, 2025 21:18
@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch 2 times, most recently from 9ced4b0 to 47e3c91 Compare April 25, 2025 13:39
@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from 47e3c91 to c2f18b7 Compare April 25, 2025 13:40
@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from acfb275 to 94f1c4d Compare April 25, 2025 16:23
@VihasMakwana
Copy link
Contributor Author

@djaglowski The testing should be good now. archive_tests is a completely separate package and it doesn't check internals. Please take a look when you can!

@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from f8aea3a to eeafee1 Compare April 28, 2025 14:14
@VihasMakwana
Copy link
Contributor Author

@djaglowski

If I'm not mistaken, when you say it's still trying to cleverly emulate them, you're referring to the calculations we perform in the tests, like i%pollsToArchive and we keep track of fingerprints as if we're doing internal tests, right?

I've taken this into consideration and reworked the approach. We now simulate a poll cycle and expect fingerprints to match. Please take a look when you can.
I've added a new test, TestArchiveRollOver, which verifies the behavior when more data is written than the archive is capable of storing. In this case, older data should be evicted.

@VihasMakwana VihasMakwana force-pushed the move-archive-separate branch from eeafee1 to 70d561d Compare April 28, 2025 14:19
@VihasMakwana VihasMakwana requested a review from djaglowski April 28, 2025 14:19
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 for iterating. It's so simple even I can understand it now.

@djaglowski
Copy link
Member

@andrzej-stencel PTAL

Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

Only some nits about logging. It's good to merge from my perspective.

Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

Thank you!

@andrzej-stencel andrzej-stencel merged commit b9b6e90 into open-telemetry:main Apr 29, 2025
173 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 29, 2025
vincentfree pushed a commit to ing-bank/opentelemetry-collector-contrib that referenced this pull request May 6, 2025
…telemetry#39353)

#### Description
Move archive into a separate package to make code more testable and less
complicated overall.
This also changes function `instantiateTracker` to
`instantiateTrackerAndArchive` a little bit to initialize the archive
and pass it to the tracker for writing older offsets.

This PR also drops some of the edge cases to reduce the complexity. I'll
work on them later

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Relates
open-telemetry#38056
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.

5 participants