This repository has been archived by the owner on Oct 29, 2021. It is now read-only.
InfluxDBStore: add proper point-batching support based on memory constraint #157
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emidoots
force-pushed
the
sg/5-influx-batching
branch
from
e9e646b to
3417fb2
Compare
| func (in *InfluxDBStore) Close() error { | ||
| close(in.flusherStopChan) | ||
| if err := in.flush(); err != nil { | ||
| return err |
There was a problem hiding this comment.
should Close return an error if flush fails? I'd expect it to have a similiar behaviour as above, and just cause a log. Also as this stands, if the flush fails, the server doesn't close.
|
A few inline comments, otherwise LGTM |
traceapp: add trace permalinks which encode the trace within the URL
…onfig around / within the InfluxDBStore struct directly.
Note: `conf.Data.QueryLogEnabled = false` is intentionally removed. It is not needed because it is the default already.
- Introduced by `094b22` - `i` was renamed to `rowIndex` in that commit, but `results[i].Slowest = append(results[i].Slowest, id)` was not updated to reflect this. Confirmed fix by visiting the Dashboard page (no more panic).
What these fields mean: - In [PR #113](#113) both `FlushTimeout` and `MaxQueueSize` were added as fields to `ChunkedCollector`. - `MaxQueueSize` effectively imposes a memory constraint on the queue such that it cannot grow above the given size, ever. In the event that it tries to (i.e. Flush / the underlying Collector is not collecting quick enough), the entire queue is dropped within `ChunkedCollector.Collect`. - `FlushTimeout` gives an assurance that the `Flush` operation does not exceed a given timeframe. This is useful because when stopping a `ChunkedCollector` via `Stop` you typically also want to call `Flush` prior (for a graceful shutdown). Why the change: - As stated above `FlushTimeout` is good for the scenario of graceful shutdowns of a `ChunkedCollector`. However, for the case of preventing memory buildup, it does not provide much usefulness (rather, `MaxQueueSize` addresses that). - The prior default for `FlushTimeout` is too over-zealous because it doesn't account for network latency, data marshaling, etc. In the case of `InfluxDBStore` this becomes even more apparent. - Change the default to a nice, loose, default value of 2s such that `FlushTimeout` is really only providing a guarantee that `Flush` in the case of graceful shutdown does eventually happen if the underlying `Collector` isn't responding. Again, for the case of preventing memory buildup, `MaxQueueSize` exists works well.
…ushTimeout default change
Because this log is currently always initialized to os.Stderr and cannot be nil.
…eturning otherwise we potentially do not close the store.
emidoots
force-pushed
the
sg/5-influx-batching
branch
from
3417fb2 to
c9dba40
Compare
|
Merged into |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ChunkedCollector.FlushTimeoutdefault from 50ms to 2s (existing applications will still work, not a backwards-incompatible change).Based on #156 for simplicity.