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

Refactor: Move queue management logic into outputController, clarify language around pipeline notifications #35078

Merged
merged 20 commits into from
Apr 18, 2023

Conversation

faec
Copy link
Contributor

@faec faec commented Apr 13, 2023

This is an internal Beats refactor and should cause no change in behavior. It is a preparation for #34396, but the functional changes are split out into a followup PR to simplify review / testing. The core change needed to support the proxy queue and the shipper is to defer queue creation until after the output configuration is received, which under Agent typically happens only after the pipeline is already created, whereas initialization logic has previously assumed that the queue could be created and used independently before knowing the output configuration.

To prepare for this change, this PR moves the queue management logic out of Pipeline and into outputController. It also renames several related fields and structures to clarify their purpose in the pipeline. The changes here are:

  • Move the queue field from Pipeline to outputController. Instead of creating a queue factory in pipeline.LoadWithSettings and instantiating it to create the queue in pipeline.New, these functions now create a queueConfig structure that is passed to newOutputController, and the output controller itself creates the queue directly. (Currently this is still done on initialization, to avoid behavior changes in this PR.) When new clients connect to the pipeline, it now fetches the queue producer from outputController instead of creating one directly (this is the hook that will allow clients to block until the queue is ready in the followup).
  • Remove the queue.ACKListener interface and replace it with a simple callback. This interface had only one function, and making it an interface created awkward reference dependencies between the queue and the Pipeline object. This change made the API slightly simpler and allowed acknowledgment bookkeeping to be handled via a simple callback field in the queueConfig structure.
  • Rename various structures / fields used in pipeline notifications to be more explicit about their function, primarily:
    • Eventer -> ClientListener. This interface is used to track client handling of events as well as client shutdown stages.
    • ACKer -> EventListener. This interface is used to track events entering and leaving the pipeline.
      • In particular (ACKer).Close() is now (EventListener).ClientClosed(), since Close gave the confusing impression of performing a close operation, when in fact it was used to notify listeners that the pipeline client was closing.

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.

How to test this PR locally

Since this involved no external behavior change, it is already exercised by the existing tests (a few needed to be updated to handle the new data structures). However, the most important things to test are that beats continue to start up successfully both standalone and under agent. These tests can use simple file or stdout-based output, and any events reaching the output are a successful test -- since the queue is created only once, and data cannot flow without it, any problems with this PR should manifest immediately on startup.

@faec faec added cleanup Team:Elastic-Agent Label for the Agent team labels Apr 13, 2023
@faec faec requested a review from fearful-symmetry April 13, 2023 14:11
@faec faec self-assigned this Apr 13, 2023
@faec faec requested review from a team as code owners April 13, 2023 14:11
@faec faec requested review from leehinman and removed request for a team April 13, 2023 14:11
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Apr 13, 2023
@mergify
Copy link
Contributor

mergify bot commented Apr 13, 2023

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @faec? 🙏.
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 commented Apr 13, 2023

💚 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

  • Start Time: 2023-04-17T21:24:43.128+0000

  • Duration: 141 min 32 sec

Test stats 🧪

Test Results
Failed 0
Passed 29388
Skipped 2211
Total 31599

💚 Flaky test report

Tests succeeded.

🤖 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!)

@faec
Copy link
Contributor Author

faec commented Apr 13, 2023

The failing test is flaky/unrelated and was supposed to be disabled, I have a PR out to correct it #35083

@faec
Copy link
Contributor Author

faec commented Apr 17, 2023

/test

2 similar comments
@faec
Copy link
Contributor Author

faec commented Apr 17, 2023

/test

@faec
Copy link
Contributor Author

faec commented Apr 17, 2023

/test

Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

LGTM

@faec faec merged commit 78dc664 into elastic:main Apr 18, 2023
@faec faec deleted the beats-startup branch April 18, 2023 00:30
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants