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

[libbeat] Cache processor docs and memory fixes. #38561

Merged
merged 22 commits into from
Apr 5, 2024
Merged

Conversation

marc-gr
Copy link
Contributor

@marc-gr marc-gr commented Mar 22, 2024

Proposed commit message

  • Change cache processor documentation from write_period to write_interval.
  • Fix expiries heap cleanup on partial file writes: popping the expiries heap completely on every partial write caused a panic on the following put operations at the time of expiry check.
  • Fix expiries infinite growth when large TTLs and recurring keys are cached: when TTLs are large and keys are recurrent, we kept track of an arbitrary amount of expiry heap entries for the same key, causing memory to grow.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@marc-gr marc-gr added the bugfix label Mar 22, 2024
@marc-gr marc-gr requested a review from efd6 March 22, 2024 15:25
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 22, 2024
Copy link
Contributor

mergify bot commented Mar 22, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @marc-gr? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

@elasticmachine
Copy link
Collaborator

@elasticmachine
Copy link
Collaborator

@marc-gr marc-gr added the Team:Security-Service Integrations Security Service Integrations Team label Mar 22, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 22, 2024
@marc-gr marc-gr added needs_team Indicates that the issue/PR needs a Team:* label backport-v8.11.0 Automated backport with mergify backport-v8.12.0 Automated backport with mergify backport-v8.13.0 Automated backport with mergify labels Mar 22, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 22, 2024
@botelastic
Copy link

botelastic bot commented Mar 22, 2024

This pull request doesn't have a Team:<team> label.

@marc-gr marc-gr marked this pull request as ready for review March 22, 2024 15:30
@marc-gr marc-gr requested a review from a team as a code owner March 22, 2024 15:30
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

andrewkroh
andrewkroh previously approved these changes Mar 22, 2024
@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 22, 2024

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 131 min 39 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@marc-gr
Copy link
Contributor Author

marc-gr commented Mar 23, 2024

/test

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

I think interval is better term here. Also see this comment from the addition. Suggest changing the documentation rather than the code.

efd6
efd6 previously requested changes Mar 24, 2024
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

(hold for discusssion)

@andrewkroh andrewkroh dismissed their stale review March 24, 2024 22:21

++ for interval

@marc-gr
Copy link
Contributor Author

marc-gr commented Mar 25, 2024

I think interval is better term here. Also see this comment from the addition. Suggest changing the documentation rather than the code.

That was my initial option, too. The thing that made it for period at the end was mainly that anyone using this setting will have it set up as write_period, and by fixing it in that direction would fix functionality for free instead of requiring action of fixing their configurations. If we are okay with this, I am fine fixing the docs instead.

@marc-gr marc-gr changed the title [libbeat] Fix write_period option name to match documentation [libbeat] Change cache processor documentation from write_period to write_interval. Mar 25, 2024
@marc-gr marc-gr requested a review from efd6 March 26, 2024 10:11
@efd6
Copy link
Contributor

efd6 commented Mar 26, 2024

Removed linter changes, not sure if we just want to ignore them or adding the nolint is preferred over fixing them.

I would prefer to just ignore them, but Andrew may disagree.

Note about the tests suggested, changed last step to see a change in the heap root, since heap Fix will not "properly" sort the slice, as it will only take care of the heap invariant !h.Less(j, i) for 0 <= i < h.Len() and 2i+1 <= j <= 2i+2 and j < h.Len() and the suggested change did not reflect in any changes in the underlying slice order.

Heap sort is not lexical sort, so this is fine. We don't care about the order of the heap entries, just that the heap behaves like a heap.

@efd6
Copy link
Contributor

efd6 commented Mar 26, 2024

/test

@efd6 efd6 dismissed their stale review March 27, 2024 00:06

Concerns addressed.

@marc-gr
Copy link
Contributor Author

marc-gr commented Mar 28, 2024

/test

1 similar comment
@efd6
Copy link
Contributor

efd6 commented Apr 1, 2024

/test

@andrewkroh
Copy link
Member

I think the failures in metricbeat-pythonIntegTest are caused by #38539.

@andrewkroh andrewkroh removed backport-v8.11.0 Automated backport with mergify backport-v8.12.0 Automated backport with mergify labels Apr 5, 2024
@andrewkroh andrewkroh merged commit b1e4abc into main Apr 5, 2024
176 of 185 checks passed
@andrewkroh andrewkroh deleted the fix/cache-docs branch April 5, 2024 19:03
mergify bot pushed a commit that referenced this pull request Apr 5, 2024
Change cache processor documentation from write_period to write_interval.

Fix expires heap cleanup on partial file writes: popping the expiries heap completely on every partial write caused a panic on the following put operations at the time of expiry check.

Fix expires infinite growth when large TTLs and recurring keys are cached: when TTLs are large and keys are recurrent, we kept track of an arbitrary amount of expiry heap entries for the same key, causing memory to grow.

(cherry picked from commit b1e4abc)
zeynepyz pushed a commit to zeynepyz/beats that referenced this pull request Apr 7, 2024
Change cache processor documentation from write_period to write_interval.

Fix expires heap cleanup on partial file writes: popping the expiries heap completely on every partial write caused a panic on the following put operations at the time of expiry check.

Fix expires infinite growth when large TTLs and recurring keys are cached: when TTLs are large and keys are recurrent, we kept track of an arbitrary amount of expiry heap entries for the same key, causing memory to grow.
andrewkroh pushed a commit that referenced this pull request Apr 8, 2024
…es. (#38745)

Change cache processor documentation from write_period to write_interval.

Fix expires heap cleanup on partial file writes: popping the expiries heap completely on every partial write caused a panic on the following put operations at the time of expiry check.

Fix expires infinite growth when large TTLs and recurring keys are cached: when TTLs are large and keys are recurrent, we kept track of an arbitrary amount of expiry heap entries for the same key, causing memory to grow.

(cherry picked from commit b1e4abc)

---------

Co-authored-by: Marc Guasch <[email protected]>
@marc-gr
Copy link
Contributor Author

marc-gr commented Apr 17, 2024

@Mergifyio backport 8.12

Copy link
Contributor

mergify bot commented Apr 17, 2024

backport 8.12

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 17, 2024
Change cache processor documentation from write_period to write_interval.

Fix expires heap cleanup on partial file writes: popping the expiries heap completely on every partial write caused a panic on the following put operations at the time of expiry check.

Fix expires infinite growth when large TTLs and recurring keys are cached: when TTLs are large and keys are recurrent, we kept track of an arbitrary amount of expiry heap entries for the same key, causing memory to grow.

(cherry picked from commit b1e4abc)
cmacknz pushed a commit that referenced this pull request Apr 18, 2024
…39001)

* [libbeat] Cache processor docs and memory fixes. (#38561)

Change cache processor documentation from write_period to write_interval.

Fix expires heap cleanup on partial file writes: popping the expiries heap completely on every partial write caused a panic on the following put operations at the time of expiry check.

Fix expires infinite growth when large TTLs and recurring keys are cached: when TTLs are large and keys are recurrent, we kept track of an arbitrary amount of expiry heap entries for the same key, causing memory to grow.

(cherry picked from commit b1e4abc)

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.13.0 Automated backport with mergify bugfix Team:Security-Service Integrations Security Service Integrations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants