Initial implementation of WAL in queued_retry, backed by diskqueue #3235
Conversation
pmm-sumo
force-pushed
the
queued-retry-wal
branch
from
dad629c to
19d2451
Compare
It is based on Jager's BoundedQueue interface and backed by go-diskqueue for WAL
pmm-sumo
force-pushed
the
queued-retry-wal
branch
from
19d2451 to
996a7fe
Compare
| @@ -185,6 +197,10 @@ func (be *baseExporter) wrapConsumerSender(f func(consumer requestSender) reques | |||
| be.qrSender.consumerSender = f(be.qrSender.consumerSender) | |||
| } | |||
|
|
|||
| func (be *baseExporter) setSignalType(signalType string) { | |||
There was a problem hiding this comment.
it seems like this is always called after newBaseExporter. Should we just make it part of that function signature?
There was a problem hiding this comment.
This does not seem to be using the recently added storage extension. I thought we were planning to use that. Any reason for not using it? If something is not good enough in the storage can we improve it?
I'd rather have one component deal with all files Collector writes so that there is one config setting to specify the directory.
Yes, this something I wanted to bring during the SIG but we ran out of the time. I would actually prefer using Storage Extension as it would allow to have much more flexibility (which would make it easier to provide capability of disk-based buffering for items currently processed by consumers). However, the extension it is currently in |
@pmm-sumo I am open to moving Storage extension to core if we can demonstrate it works nicely for queued_retry. It will be a strong proof that it is the right approach because it handles nicely two quite independent cases. We will need to demonstrate that it works well functionally and from performance perspective. On the other hand if we try and see that it is not working well for queued_retry we need to either go back to the drawing board and improve it or make it clear that queued_retry has significantly different storage needs and we do not aim one Storage extension to be suitable for all use case (also a valid outcome, but I want it to be explicitly researched and concluded). It would be great if you could do this research and post your conclusions. Let me know if you want to discuss live. |
|
@tigrannajaryan very good point, I should have an alternative version based on Storage extension tomorrow, will be more than happy to discuss. Let me post an update here and we can sync on Slack to setup a live session |
pmm-sumo
changed the title
|
Took a bit longer, but here's an alternative approach based on file storage extension: #3274 |
pmm-sumo
changed the title
|
Closing in favor of file storage extension based approach |
Description:
PoC for WAL implementation within queued_retry, based on Jager's BoundedQueue interface (providing a simple replacement) and backed by go-diskqueue for WAL.
It has some limitations (mentioned in the README.md and through several placeholders in code) but perhaps it would be good to check with the community if this is the right direction.
Currently, it only stores the batch, but the idea is to include context as well (though only
client.Client).Initially, I based on prometheusremotewrite WAL implementation, hoping we might have a common part here. However it turned out that
tidwalused underneath has several limitations (like expectation there's at least one item always present) and switched togo-diskqueueunderneath, which also seems to have large user base (judging by number of stars and number of forks) and no dependencies.There are also some things I would like to make nicer (like how requestUnmarshaler is being passed) and few tests to fix/add
Link to tracking Issue: #2285
Testing: Unit Tests and manual testing, more to come
Documentation: README.md updated, more to come