Skip to content

Add entry IDs to the memory queue#32541

Merged
faec merged 29 commits intoelastic:mainfrom
faec:queue-batch
Aug 10, 2022
Merged

Add entry IDs to the memory queue#32541
faec merged 29 commits intoelastic:mainfrom
faec:queue-batch

Conversation

@faec
Copy link
Copy Markdown
Contributor

@faec faec commented Jul 28, 2022

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 cmacknz and rdner and removed request for a team July 28, 2022 20:59
@elasticmachine
Copy link
Copy Markdown
Contributor

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 fearful-symmetry and leehinman and removed request for cmacknz and rdner July 28, 2022 20:59
@faec
Copy link
Copy Markdown
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
Copy Markdown
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.


testForward := func(q queue.Queue) {
waiter := &producerACKWaiter{}
producer := q.Producer(queue.ProducerConfig{ACK: waiter.ack})
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

cmacknz commented Aug 9, 2022

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

Copy link
Copy Markdown
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.

}

func (batch *diskQueueBatch) ID(i int) queue.EntryID {
return 0
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement 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.

5 participants