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

Add entry IDs to the memory queue #32541

Merged
merged 29 commits into from
Aug 10, 2022
Merged

Add entry IDs to the memory queue #32541

merged 29 commits into from
Aug 10, 2022

Conversation

faec
Copy link
Contributor

@faec faec commented Jul 28, 2022 •
edited
Loading

What does this PR do?

Adds an incrementing ID to events in the memory queue, as described in elastic/elastic-agent-shipper#27. This is needed by the shipper, to report to inputs whether their events have been sent yet.

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.

Related: elastic/elastic-agent-shipper#27

@faec faec added enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Jul 28, 2022
@faec faec self-assigned this Jul 28, 2022
@faec faec requested a review from a team as a code owner July 28, 2022 20:59
@faec faec requested review from rdner and cmacknz and removed request for a team July 28, 2022 20:59
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@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 Jul 28, 2022
@faec faec requested review from leehinman and fearful-symmetry and removed request for rdner and cmacknz July 28, 2022 20:59
@faec
Copy link
Contributor Author

faec commented Aug 2, 2022

Added some more bits around metrics reporting, since I realized this code reported event IDs to the producers and consumers but the shipper also needs a standalone call to get the current ID, which I've added to the metrics struct already provided by the memory queue.

@faec
Copy link
Contributor Author

faec commented Aug 3, 2022

The existing tests pass, the new tests for the event ID feature still need work:

  • they involve a time.Sleep which is undesirable. Not sure yet if I can remove it, though, since the memory queue propagates ACKs asynchronously.
  • some of them don't pass yet, particularly with the buffered version. I'm debugging that now, not 100% sure yet whether the problem is in the implementation or tests.

@jlind23 jlind23 mentioned this pull request Aug 8, 2022
Closed
cmacknz
cmacknz reviewed Aug 9, 2022
View reviewed changes
libbeat/publisher/queue/memqueue/queue_test.go

testForward := func(q queue.Queue) {
waiter := &producerACKWaiter{}
producer := q.Producer(queue.ProducerConfig{ACK: waiter.ack})
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth having a test case for concurrent producers?

@cmacknz
Copy link
Member

cmacknz commented Aug 9, 2022

LGTM, I'll wait for @rdner to confirm that his comments have been addressed before approving.

@cmacknz
Copy link
Member

cmacknz commented Aug 9, 2022

Would also be good to get @leehinman to look at this, I've pinged him on Slack.

leehinman
leehinman approved these changes Aug 9, 2022
View reviewed changes
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.

In a follow on PR could we get a doc with some high level design docs/diagrams.

libbeat/publisher/queue/diskqueue/consumer.go
return batch.frames[i].event
}

func (batch *diskQueueBatch) ID(i int) queue.EntryID {
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct in assuming a "real" event ids for the disk queue would be in a follow on PR? If so can we open an issue and make sure it is marked as "related" in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the issue is elastic/elastic-agent-shipper#27

@faec faec merged commit dd118ef into elastic:main Aug 10, 2022
@faec faec mentioned this pull request Aug 15, 2022
Merged
This was referenced Aug 22, 2022
Closed
Closed
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
@faec @chrisberkhout
Add entry IDs to the memory queue (#32541)
300f3d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmacknz cmacknz cmacknz left review comments

@leehinman leehinman leehinman approved these changes

@rdner rdner rdner approved these changes

@fearful-symmetry fearful-symmetry Awaiting requested review from fearful-symmetry

@belimawr belimawr Awaiting requested review from belimawr

Assignees

@faec faec

Labels
enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@faec @elasticmachine @cmacknz @rdner @leehinman