Skip to content
This repository has been archived by the owner on Sep 21, 2023. It is now read-only.

Add package and API skeleton for internal queue #40

Merged
merged 7 commits into from
May 26, 2022
Merged

Conversation

faec
Copy link
Contributor

@faec faec commented May 20, 2022

This is the first pass of integrating the libbeat queue into the shipper server. It creates the package namespace and structures and passes through the initialization / shutdown process, but doesn't yet use a real queue internally (that is waiting on the libbeat-side api changes to be merged in elastic/beats#31699). But this creates a skeleton and moves things to the right places so we can keep building around it without version skew from the draft PRs.

@faec faec added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label May 20, 2022
@faec faec requested a review from a team as a code owner May 20, 2022 15:48
@elasticmachine
Copy link
Collaborator

elasticmachine commented May 20, 2022

💚 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: 2022-05-26T16:56:20.698+0000

  • Duration: 8 min 27 sec

Test stats 🧪

Test Results
Failed 0
Passed 6
Skipped 0
Total 6

🤖 GitHub comments

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

  • /test : Re-trigger the build.

queue/queue.go Outdated Show resolved Hide resolved
@cmacknz
Copy link
Member

cmacknz commented May 20, 2022

Looks good as a starting point. Tests are coming later I assume?

@faec
Copy link
Contributor Author

faec commented May 20, 2022

Looks good as a starting point. Tests are coming later I assume?

Yeah, I just wanted to lock in the structure that isn't dependent on the outstanding libbeat api change, I'm expecting some unit tests when that gets merged and probably additional end-to-endish ones following that to test the whole process.

package queue

import (
beatsqueue "github.com/elastic/beats/v7/libbeat/publisher/queue"
Copy link

Choose a reason for hiding this comment

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

Why are you using this? Is this a temporary import and going to be removed in a later relase?
I was under the impression that we are trying to avoid importing from beats in our new repositories. If we want to reuse something from beats we should either move it to elastic-agent-libs or copy it to the new repo where it is used.

Copy link
Member

Choose a reason for hiding this comment

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

The shipper is initially going to be built out of the beats outputs, queues, and the code that glues them together. That is a large chunk of code to move that at this point would just slow down development. I would like us to prioritize getting a working shipper prototype over the code reorganization right now.

I am in favour of moving all that code into elastic-agent-libs afterwards or perhaps in the background. That will make it clear that the code is used outside of just beats now.

I will create an issue to migrate the code we end up using out of beats, which I assume will also be easier once we know what pieces we need to reuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there is still more pending work to clean up and modularize the queue and I expect that as that progresses it will probably migrate to elastic-agent-libs, it just wasn't part of the first draft.

Copy link
Member

Choose a reason for hiding this comment

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

#43

@faec faec merged commit ca42ed1 into main May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants