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.

Sign up for GitHub

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

Jump to bottom

[chore][pkg/stanza] fileconsumer: integrate recently added fileset #30728

Merged
merged 16 commits into from
Feb 4, 2024
Merged

[chore][pkg/stanza] fileconsumer: integrate recently added fileset #30728

merged 16 commits into from
Feb 4, 2024

Conversation

VihasMakwana
Copy link
Contributor

Description: Integrate recently added fileset

@VihasMakwana VihasMakwana requested a review from a team January 23, 2024 12:12
@VihasMakwana VihasMakwana force-pushed the integrate-tracker branch 2 times, most recently from a630205 to 708eaea Compare January 23, 2024 12:35
@VihasMakwana VihasMakwana requested a review from atoulme as a code owner January 23, 2024 12:40
@djaglowski djaglowski changed the title [chore][pkg/stanza] filecomsmer: integrate recently added fileset [chore][pkg/stanza] fileconsumer: integrate recently added fileset Jan 23, 2024
djaglowski
djaglowski reviewed Jan 23, 2024
View reviewed changes
pkg/stanza/fileconsumer/file_other.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Show resolved Hide resolved
pkg/stanza/fileconsumer/file_windows.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
djaglowski
djaglowski reviewed Jan 26, 2024
View reviewed changes
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file_test.go Outdated Show resolved Hide resolved
@VihasMakwana VihasMakwana force-pushed the integrate-tracker branch 2 times, most recently from 23c725a to 2dd023f Compare February 1, 2024 10:19
@VihasMakwana
Copy link
Contributor Author

@djaglowski up for final review.

djaglowski
djaglowski reviewed Feb 1, 2024
View reviewed changes
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.

Looking really good. A few more suggestions though.

pkg/stanza/fileconsumer/file.go Outdated
Comment on lines 50 to 51
if matches, err := m.fileMatcher.MatchFiles(); err != nil {
m.Warnf("finding files: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

This check and warning are still useful.

pkg/stanza/fileconsumer/config.go Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file_other.go Outdated Show resolved Hide resolved
pkg/stanza/fileconsumer/file_windows.go Outdated Show resolved Hide resolved
VihasMakwana and others added 3 commits February 1, 2024 22:11
@VihasMakwana @djaglowski
Update pkg/stanza/fileconsumer/file.go
6e39722 
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
fix: refactor
af07a59
chore: add check
2be9d53
djaglowski
djaglowski approved these changes Feb 2, 2024
View reviewed changes
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.

Looks good. Thanks @VihasMakwana

pkg/stanza/fileconsumer/file.go Outdated Show resolved Hide resolved
@djaglowski djaglowski added the Run Windows Enable running windows test on a PR label Feb 2, 2024
@djaglowski
Copy link
Member

Windows compilation is failing: pkg/stanza/fileconsumer/file.go:174:15: not enough arguments in call to m.preConsume

@VihasMakwana
Copy link
Contributor Author

@djaglowski guess we can merge this. CI's green.

@djaglowski djaglowski merged commit 416f296 into open-telemetry:main Feb 4, 2024
150 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 4, 2024
@djaglowski djaglowski mentioned this pull request Mar 27, 2024
Closed
@sbylica-splunk
Copy link

Hi @djaglowski, I took over work on parallel file consumer after @VihasMakwana left the company, would it be possible to set up some kind of meeting so I can get up to speed on the current status of this feature?

@djaglowski
Copy link
Member

Hi @sbylica-splunk, this PR was merged so I'm not sure what you're referring to.

@sbylica-splunk
Copy link

Hi @djaglowski , we have a task that reads as:

Break down work on the thread pool model.
Part 1: Add trie and test cases in core.
Part 2: Discussion with Daniel Jaglowski regarding caveats.
Part 3: Roll out PRs for different aspects of filelog threadpool feature.

I wanted to reach out to you, and ask if there is anything that needs to be done on our side since @VihasMakwana left the company, and you were mentioned in this topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djaglowski djaglowski djaglowski approved these changes

@atoulme atoulme Awaiting requested review from atoulme atoulme is a code owner

Assignees

@bogdandrutu bogdandrutu

Labels
pkg/stanza receiver/filelog receiver/otlpjsonfile Run Windows Enable running windows test on a PR
Projects
None yet
Milestone
v0.94.0
Development

Successfully merging this pull request may close these issues.
4 participants
@VihasMakwana @djaglowski @sbylica-splunk @bogdandrutu