If the inner function that handles the open of storage and cache
fails, we close the fragment. If we closeStorage() before that,
then we can try to close the storage again, which causes a panic
when we try to mark the generation as Done again.
I was going to set f.gen = nil after marking it done, but I'm
not feeling safe about that -- there's too many places where
we check things about f.gen, and it seems unsafe. The generation
code should be removed at some point, because it all exists
as a workaround for not having any way to detect when reads are
"done", because we didn't want to do something huge and intrusive,
like adding the Tx system and requiring transactions to get
closed.
Performance of tests on MacOS has been atrocious for a while, and
a lot of that is fsync, so we're trying to make that optional.
To test all of this, I modified RBF to panic if anything tried to
open an RBF database without disabling fsync, and ran the tests that
way, and tracked down the various places this could still happen.
There's a lot of places in our tree where we were creating
test holders which were not getting created with fsync disabled, which
results in a surprisingly large number of points at which we end
up calling fsync in tests, which makes tests much slower than they
need to be. There's also a bunch of places where the flags don't get
propagated correctly; for instance, storage.fsync didn't propagate
to the RBFConfig.
We add an "fsync enabled" flag to OpenTranslateStoreFunc, so we can
tell translation stores that we don't need syncing, so the server's
config can be passed on appropriately.
More of the test code that sets things up is correctly configuring
that flag by default.
We also change the barely-used bolt storage backend to support this as
well.
With this done, the only calls to fsync left in a run of `go test -short`
in the top-level directory are from the zap logger in etcd, and consumed
around 0.03 seconds. The overall impact is that `go test -short`
went from "takes enough more than 10 minutes that i don't know how long
it takes" to about 2.5 minutes.
boltdb has a couple of places where it fsyncs even when fsync is
disabled, this turns out to cost an amazing amount of time over
several thousand databases in our test run. In theory, they are
rare circumstances compared to updates; in practice, when you
open 256 partition key translation databases per server opened
and most of them never get written to, not so much.
In test runs, we open, and close, *huge* numbers of databases. Even
the single fsync on close for these ends up being expensive on some
hosts. *cough* Apple. At least in theory, writes delivered to the
disk are just as written whether or not you've hit fsync, as long
as the machine doesn't power off before getting to them. In the
circumstances where we disable fsync, that's fine.
Since we already have an fsync function for "fsync if it's
not disabled", use that.
We disable fsync more consistently in testing, including using
etcd's already-existing UnsafeNoFsync option to disable fsyncs
in the backing store boltdb used by etcd, to reduce runtime of
our tests on MacOS significantly.
Corresponding to this, we update etcd by one patch to pick
up a locally-invented patch which turns out to be nearly-identical
to the upstream fix for "disabling fsync makes boltdb not
even bother to write some data sometimes", which caused crashes
galore.
This adds the following test:
1. cluster comes up (node 1,2,3), status normal
2. Pause node 3
3. Insert keys making sure to filter out the keys that will go to the paused node
4. Wait for status to become degraded
5. Unpause node 3
6. Wait for status to get back to normal
7. Check that keys were replicated to all 3 nodes
The actual code here is mostly jaffee's, but I've reworked it some.
This doesn't directly seem to be using UnionInPlace, but really it
is.
The actual logic inside (*Row).Union is a mess and probably silly
in a few ways, but hardly matters. The important part is that,
instead of calling it once per child as we get them, we gather
all of them at once and then call it on all of them. That gets
us a call to (*Row).Union that does a very elaborate dance to
compute a call to (*rowSegment).Union on the only segment present
in each of those rows, which then does a simpler thing to
call (*Bitmap).Union() with the first response as a receiver
and the rest as parameters, and THAT then ends up calling either
unionIntoTargetSingle() if there's only one other bitmap,
or using UnionInPlace on a Freeze() of the first bitmap, which
gets us (we hope) the benefits of the fancy UnionInPlace logic.
Every part of this is a reminder that we really need to replace
roaring and also the Row/rowSegment stuff some day.
I assumed the existing import code handled replicas. It doesn't, actually.
It just assumes they're handled. So, in the new import code, when splitting
things up by-shard, send each shard's data to *every* node that has
that shard, not just the first one.
It was useful having this in the package to verify code coverage of
the translator, but that having been verified, I'd sort of rather have
it NOT live in the package at all, it's really a testing-only kind
of thing.
We add a new protobuf type. Also, protoc changed slightly and remade
some tests, in a way which should have no effects but makes the code
*very* slightly cleaner.
This introduces the first testing code in encoding/proto (whoops)
so that scaffolding is a first draft; if you're looking at this code
and the design is a problem go ahead and fix it.
The purpose of this is to verify that we're actually covering all
the branches in the ingest.ShardedRequest and pb.ShardedIngestRequest
message conversions. (Except the top-level one for a nil request,
which isn't checked by this.)
The coverage report doesn't actually include coverage for the ingest
code, though, so we haven't actually properly tested Compare.
Baby steps!
We add endpoints and protobuf encode/decode to allow for sending
sharded requests over the wire in protobuf, so we can take our
sharded data and send it to other nodes if needed.
This is a squash of >15 other commits, so a bit of history
is relevant:
The Request type had FieldTypes in it because the field type
information was needed for sharding because sorting requires
that information. We change this around to make the external
sharding operation require the field types, and curry that
through the codec -- the codec is needed to tell the request
how it shards. (This is because the correct sorting order
varies by field type.) Requests (and ShardedRequests) no
longer have that table in them.
And then we hit a nasty bug in production and RCA showed
that our testing wasn't good enough and we need to be more
careful, and I discovered that test coverage in this package
was around 70%.
So, the other big thing here is coverage testing; in order to
make coverage testing viable and programmatically testable,
we have added the ability to render requests *back* to
JSON. This is not a great idea, but it does allow us to do
a lot of sanity-checking and verify that the encodings we're
using are consistent and correct.
This, plus some specific tests of decoding specific flawed
inputs, has caught a number of issues. Which are now fixed!
A lot of internal API surface got slightly changed, in ways
that make it simpler to work with. For instance, the
(*FieldOperation).TranslateUnsigned function doesn't really
need to exist; we can just have a non-method translate
function for unsigned and for signed, and use them based on
field type.
The stable translation hack used for testing had a bug that
could allow it to end up producing incorrect results if you
asked it to translate an ID first rather than exclusively
asking it to translate strings first, this has been
corrected. (This is a bug fix in code that was added
partway through creating this, but is tricky enough to
mention its own comment.)
Test coverage is now just over 90%, and a lot of what's left
is error-check returns that may well be actually unreachable
unless, say, the documentation for encoding/json is full of
lies. Which it probably is.
