[libbeat] Disk queue implementation #21176
Conversation
|
Pinging @elastic/integrations (Team:Integrations) |
| func SettingsForUserConfig(config *common.Config) (Settings, error) { | ||
| userConfig := userConfig{} | ||
| if err := config.Unpack(&userConfig); err != nil { | ||
| return Settings{}, err |
There was a problem hiding this comment.
There's a lot of inconsistent error return styles here. Sometimes we wrap an error, sometimes we don't. Unless something else is requesting a specific error type, we might want to just use errors.wrap or fmt.Errorf for everything.
There was a problem hiding this comment.
The heuristic I aim for is to ask whether the error being returned is understandable in the context of the caller. For example, errors in the deleter loop aren't wrapped, because they are reported (and grouped / wrapped) when they get back to the core loop, and it looks silly to have a message that looks like "couldn't delete segment file: [couldn't delete segment file: [actual error]]". So for low-level helpers I often leave it unwrapped, knowing that the caller is responsible for reporting it comprehensibly.
That said, config.go doesn't seem to follow that convention, and could use a little more verbosity in the messages, so I fixed the calls here :-)
|
I think I understand most of this. Good work! |
|
Updates:
|
faec
changed the title
| dq.handleProducerWriteRequest(request) | ||
|
|
||
| // The request inserts 100 bytes into an empty queue, so it should succeed. | ||
| // We expect: |
There was a problem hiding this comment.
Nit: describing the expectations in the comment do not provide value as the messages passed to t.Error functions tell the same story.
There was a problem hiding this comment.
It's technically redundant, but the verbal description is shorter and easier to parse. I also like having a description of what I'm about to check, because the two listed invariants are logically equivalent to the 5 conditions in the code, but a reader seeing the test for the first time only knows what the code actually tests, and not what the author thought they were testing (which isn't always the same). This way if a test fails it's easier to recognize whether the problem is in the package or the test itself (i.e. whether the test is checking the invariants incorrectly, rather than the invariants actually failing).
(The secret long-term plan is to have verbal descriptions like this for every logically distinct state change, and to coalesce them into package documentation so it's easier to see how the pieces fit together.)
Initial implementation of the new libbeat disk queue (cherry picked from commit 2b8fd7c)
* upstream/master: feat: prepare release pipelines (elastic#21238) Add IP validation to Security module (elastic#21325) Fixes for new 7.10 rsa2elk datasets (elastic#21240) o365input: Restart after fatal error (elastic#21258) Fix panic in cgroups monitoring (elastic#21355) Handle multiple upstreams in ingress-controller (elastic#21215) [CI] Fix runbld when workspace does not exist (elastic#21350) [Filebeat] Fix checkpoint (elastic#21344) [CI] Archive build reasons (elastic#21347) Add dashboard for pubsub metricset in googlecloud module (elastic#21326) [Elastic Agent] Allow embedding of certificate (elastic#21179) Adds a default for failure_cache.min_ttl (elastic#21085) [libbeat] Disk queue implementation (elastic#21176)
What does this PR do?
This PR implements a new disk-based queue for libbeat.
This is a draft PR: it is ready to start receiving feedback and rolling reviews, but some implementation details are still pending and it isn't ready for checkin.
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.How to test this PR locally
Add to the beat configuration:
The beat should operate as usual, and the event data should be stored in
data/diskqueuewhile waiting to be ingested.Release testing
While the preceding is good enough for smoke testing, we should include a few more scenarios for release prep:
data/diskqueue/NNN.segin a hex editor -- the event data blobs are JSON and can be recognized visually -- but that is optional as that level of detail belongs more in the automated tests).max_size, both small enough that the queue consistently reaches its maximum size (which can be seen by checking the files indata/diskqueue) and large or unbounded (max_size=0).queue.disk.pathsetting so the beat itself isn't on a full partition).Since the queue itself operates strictly locally, it is fine to test each of these scenarios in isolation -- we wouldn't gain much practically by testing on the powerset of the preceding conditions, so follow whatever sequence makes things simplest.