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/fileconsumer] Add trie and test cases #24982

Merged
merged 12 commits into from
Aug 8, 2023

Conversation

VihasMakwana
Copy link
Contributor

Description: Add Trie data structure and keep it separate from PR #23056

Testing: Relevant test cases added

@VihasMakwana
Copy link
Contributor Author

@djaglowski I created this new PR. The previous one will be a mess if I revert in the same PR.

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.

The tests in this PR are substantially less thorough than those I contributed in VihasMakwana#2.

Those cases were designed to give me confidence that the trie behaves as I expected. We've agreed to remove Put's ability to update fingerprints, but this should only affect some specific assertions. All of those sequences should still be tested. Will you please restore the test cases and change only the assertions directly related to the Put function's update capability?

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

@djaglowski done, sorry bout that.

@VihasMakwana
Copy link
Contributor Author

@djaglowski

  • TODO how do we know we need to call Put()?
    • If any match returns false, it would mean that it's a new file and we will call Put(). If it returns true, we would move to next files and skip this one as it will be a duplicate.

@djaglowski
Copy link
Member

  • If any match returns false, it would mean that it's a new file and we will call Put(). If it returns true, we would move to next files and skip this one as it will be a duplicate.

I agree.

I'm going through my test cases in enough detail that I'm just going to make another PR to update the assertions as I understand them now.

@VihasMakwana
Copy link
Contributor Author

  • If any match returns false, it would mean that it's a new file and we will call Put(). If it returns true, we would move to next files and skip this one as it will be a duplicate.

I agree.

I'm going through my test cases in enough detail that I'm just going to make another PR to update the assertions as I understand them now.

sounds fair, you'd wanna be sure that TRIE behaves as expected.

@VihasMakwana
Copy link
Contributor Author

I can add you to my repo, you can commit directly if you want to?

@djaglowski
Copy link
Member

That's ok. I'd prefer to do it through PR anyways 👍

@djaglowski
Copy link
Member

I made the PR. Tests passed all my expectations now. I'll take another look tomorrow with fresh eyes but I think this may be good to go.

@djaglowski
Copy link
Member

@VihasMakwana, my PR is here. It also includes some lint fixes.

@VihasMakwana
Copy link
Contributor Author

@djaglowski I think we are good to go. I believe we can skip the changelog for this, as we did for my previous PR. I will include trie in threadpool PR's changelog.
Let me know if you think otherwise.

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

I agree with skipping the changelog. Ultimately, this is an internal package and not in itself a user-facing concern. We'll add a changelog describing the user-facing behavior that it enables, when the rest of the solution is in place.

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 persisting on this @VihasMakwana. I'm looking forward to seeing how well it supports the concurrent handling of files that we're aiming for.

@djaglowski djaglowski merged commit eb26ae6 into open-telemetry:main Aug 8, 2023
94 of 95 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 8, 2023
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.

3 participants