Batched write support #1644
Batched write support #1644
Conversation
|
Most definitely work-in-progress. It compiles, but that's about it, and is not complete. The corresponding "apply" commands have yet to be coded. |
|
I have also not performed any testing, short of the new unit tests. |
otoolep
force-pushed
the
batched_series_create
branch
from
0304aee
to
404b221
Compare
| if err != nil { | ||
| return err | ||
| } | ||
| db.addSeriesToIndex(cm.Name, series) |
There was a problem hiding this comment.
@benbjohnson @pauldix -- this is in the proposed "apply batched series and field creation" code. This has a race such that if an addition of a series fails to be committed to the Metastore, any changes to in-memory indexes, due to the calls to db.addSeriesToIndex(), are not rolled back. Pre-existing code also suffered from this issue with relation to fields.
This means, in a failure scenario, we could end up with a different data in memory versus in the Metatsore. We can avoid this by cloning the in-RAM indexes for the purposes of the loop, and then swapping the modified clone and in-RAM authoritative copy at the very end of the function, assuming no errors.
Make sense? Is there another approach? Do we care?
There was a problem hiding this comment.
mustUpdate() should cause the server to crash hard so that when it comes back up it'll rebuild its in-memory index.
There was a problem hiding this comment.
Ah yes, indeed it will. Nice.
There was a problem hiding this comment.
another thing to do is to put the update of the in memory structures after the metastore update, but before it releases the database schema lock
otoolep
force-pushed
the
batched_series_create
branch
2 times, most recently
from
85628c2
to
b445457
Compare
|
This patch can be summarized by two key changes:
Best case is a batch comes in that results in a single buffer i.e. all points in the batch are destined for the same shard. This would mean a single batch is routed around the cluster in a single message, and that single batch is written to Bolt in a single transaction. |
|
@otoolep the changes lgtm. we'll need to handle batching coming from different clients as well in the future but we can do that in a separate PR. 🚢 |
| for _, p := range points { | ||
| // Local function makes lock management foolproof. | ||
| measurement, series, err := func() (*Measurement, *Series, error) { | ||
| s.mu.RLock() |
There was a problem hiding this comment.
this lock should be outside the points loop
|
A few comments, but otherwise looks good. |
Full unit tests added for happy paths.
This test passes, but only because it is checking for the wrong results. Once batching is implemented this test will fail (as long as it is unaltered).
Unit tests need updating since some tests are no longer valid.
Using maps was resulting in unpredicatable ordering of columns and tags.
Its presence is making Bolt-level batching quite awkward, and since it's not used, just remove it.
In addition, memomize the Field codecs.
otoolep
force-pushed
the
batched_series_create
branch
from
d39db6a
to
9c4174a
Compare
No description provided.