vlinsert: prevent a slow vlstorage node from stalling ingestion to the whole cluster#1616
vlinsert: prevent a slow vlstorage node from stalling ingestion to the whole cluster#1616cuongleqq wants to merge 2 commits into
Conversation
d263806 to
855609b
Compare
…de insert concurrency limit
855609b to
bbf38b2
Compare
| } | ||
|
|
||
| if !errors.Is(err, errTemporarilyDisabled) { | ||
| if !errors.Is(err, errTemporarilyDisabled) && !errors.Is(err, errConcurrencyLimitReached) { |
There was a problem hiding this comment.
Currently, when the errConcurrencyLimitReached error occurs, no error is logged. This is incorrect behavior, as it causes silent uneven distribution across storage nodes with no visibility for the user as to why this is happening. In my opinion, we need both a dedicated metric and an warn log entry in the case of re-routing pending data.
But even if we restore the log and add a metric, is this new behavior actually correct? After this PR data will be distributed unevenly across nodes if one storage node operates slower than the others. This can lead to a poor user experience - e.g., if a slower storage node retains a couple of extra hours of data, creating a false expectation that this data should be present on the other storage nodes as well, which is an incorrect assumption.
One storage node could be slower than the others not only because it has different resources, but even if it has different kernel versions, hardware issues, improper network configuration, and tens of other reasons that users have to investigate to avoid the uneven distribution issue. I believe we need to implement additional alerts to support the new behavior.
Personally, I think the old logic is simpler to understand from a user POV. The only thing we should fix is in the grabPendingDataForFlushLocked we should check if there no available pending buffers to grab - in this case we should report an error so users can start investigating the reason, while still distributing the data evenly.
There was a problem hiding this comment.
@vadimalekseev thanks for reviewing
In my opinion, we need both a dedicated metric and an warn log entry in the case of re-routing pending data.
Agreed with this one. Fixed.
Personally, I think the old logic is simpler to understand from a user POV. The only thing we should fix is in the grabPendingDataForFlushLocked we should check if there no available pending buffers to grab - in this case we should report an error so users can start investigating the reason, while still distributing the data evenly.
#1572 adds a timeout, but with the old global pool, a sick node still repeatedly stalls ingestion for the whole cluster, 1 minute (request timeout) at a time. A cluster-wide stall for a minute is a much bigger problem than slightly uneven distribution.
And the uneven distribution might not happen as often as you think. A slightly slower node rarely hits the limit. Hitting it persistently means the node cannot keep up with its share. Sending more batches to an overloaded node makes it worse. Re-routing the excess keeps the consequence local to that node only.
What do you think?
…ible and skip busy nodes when re-routing
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
2 participants
vlinsertshared a single fixed buffer pool across allvlstoragenodes, so a slow or unresponsive node could hold every buffer and stall ingestion for the whole cluster. This PR replaces the pool with a per-node cap on concurrent insert requests (-insert.concurrency), re-routing anything above the cap to less busy nodes.This is one of the two parts needed to fix #1512. The other part is a per-request send timeout, added in #1572: it makes a request to a slow or unresponsive node fail over to other nodes instead of holding its logs forever.
This PR should be merged after #1572; only both together close #1512.