From Nia:
While debugging the Q2 bugs this was somewhat useful in analyzing cluster events. As for the spammy part. . . that seems to be more of an issue with spamming our resets than an issue with the log itself.
This used to be possible to hit, but I think now that Distinct on a
set field returns a *Row rather than a SignedRow it isn't an issue. (I
wasn't able to trigger it in the tests). Adding the fix anyway as it
seems safer than not.
The rest of the changes are test infrastructure to make it easy to
call GRPC queries and verify the results as CSV.
I used a "paranoia" check to find these, but then realized the check
had a ton of false positives and doing it properly wasn't going to be
straightforward. I'm leaving the paranoia stuff in unless there are
objections, because I've wanted it before and not had it.
I also removed a log line that is very verbose and I don't think helps
anyone.
The "needs snapshot queue" check was broken, as it only checked inside a loop over indices.
If there are no indexes yet (or more likely if the indices have not yet been loaded off of disk), then this would never use the snapshot queue on roaring.
moved lonquerytime from cluster into server and moved cluster.longquerytime into top level config
kept cluster.longquerytime for backwards compatibility, favored if both longquerytime options are present
- view.openFragmentInTx was forcing a directory scan
for shards on every open fragment during Holder.Open().
Seen by pprof profile having excessive allocations
from dbshard.go listDirUnderDir().
- the -fix flag repairs replication errors by copying from the primary.
- the -fixkeys flag repairs any string key translation issues.
- make pilosa-fsck installs pilosa-fsck and builds release-pilosa-fsck.COMMIT.GOOS.tar.gz release tarbar
Previously, we never waited for translation sync goroutines to stop.
That issue should be mostly harmless in the normal path.
Additionally, this waits for the translation sync to shut down when stopping the server.
- introduce Query Context (Qcx) for managing database-per-shard.
- replaces the MultiTx, so mtx.go is retired and removed.
- introduces the HolderConfig struct and all Holders now have
a path from birth.
- rbf speedups on bitwise writes
- badgerdb is removed due to unresolvable write conflicts.
fixes#703#676
- rbf had races around the new rootRecords cache in tx
- rbf tx needed a write lock on the db now that rootRecords are written
- added a global registry for rbfDB to correctly dedup instances
- implement DeleteFragment, DeleteIndex for rbf
- use badger style keys for rbf to allow content checksumming to be list
containers in the same order
- lots of other integration of rbf into pilosa layer.
a) All tests green under -race for both PILOSA_TXSRC=roaring and PILOSA_TXSRC=badger.
b) Distinct is merged back into mainline pilosa.
Seebs notes on the Distinct work:
merge Distinct plugin back into main source tree, convert to Tx
We drop all references to the Preemptively Deprecated Don't You Dare
Use This extension interface, and move the one and only extension we had
(Distinct) into the main executor.
Also this fixes an arguable bug, which is that Container.AsBitmap()
would panic on a nil parameter, but it should have returned an empty
bitmap, because a nil *Ccontainer is a valid empty container. This
simplifies logic significantly in Distinct.
Fixes#569#570#571#572#573#584#585
- all tests green on RoaringTx
- RoaringTx on by default
- blueGreenTx testing framework available for A-vs-B comparison
of Tx implementations
- flag -tx added to server command line but not wired to
change NewIndex() selection yet.
- 918 green tests, 14 tests red on BadgerTx.
A full list of the 14 red tests on BadgerTx follows.
Note that these red tests represent not defects in BadgerDB
or BadgerTx but rather failures of the pre-existing pilosa infrastructure to yet
be fully adapted from files to using a transactional storage engine.
As such these are tests that RBF should not be expected to
pass yet either.
Fixing the pilosa infrastructure to allow these tests
to go green under Badger is the next and highest priority
order of business, but RBF can get much testing benefit
from the 918 green tests we do have, and hence we merge
as much as we have today.
The 14 red tests when NewIndex() is set to use
BadgerTx are as follows. Note in particular
that pilosa cluster resizing is not working yet under a
transactional store.
TestCluster_ResizeStates/Multiple_nodes,_with_data
TestImportClearRestart/0MaxOpN10000
TestImportClearRestart/1MaxOpN10000
TestImportClearRestart/2MaxOpN10000
TestImportClearRestart/3MaxOpN10000
TestExecutor_Execute_Existence/Row
TestExecutor_ForeignIndex
TestExecutor_Execute_CountDistinct/Distinct
TestExecutor_Execute_CountDistinct/Count(Distinct)
TestExecutor_Execute_CountDistinct/GroupBy(Distinct)
TestExecutor_BareDistinct
TestExecutor_Execute_TopNDistinct/TopN
TestHolderSyncer_IntField/BasicSync
TestHolderSyncer_IntField/MultiShard
This is sort of large, but it's annoyingly difficult to
separate out.
The basic idea is to allow us to have a single holder-iterating
block of code, which is associated with the holder, that can be used
for various things, like the snapshot queue background scan, or
for inspect operations.
We invent the concept of a HolderFilter, which is a thing that
can decide what things in a holder it cares about, and a HolderOperator,
which can also process those things selectively.
In the process, we fix up a couple of subtle bugs in the
inspect logic; specifically, the assumption that the mapped flag could
tell you whether a container was modified by the ops log doesn't
work with mmap, so we have a shiny new flag which is used to track
that, internal to the roaring/container code.
All of this leads to the actual *point* of this exercise, which is
making it easier to create an /inspect endpoint which produces almost
the same data we'd have gotten from `pilosa inspect` on a data directory;
the distinction is that it doesn't try to identify the distinction
between data from disk and data from operations since the file was
loaded. Possibly it should, but it doesn't yet.
The snapshot queue is now implemented using the HolderOperator
design, which requires some subtle changes to how it works, but
overall makes it easier to follow the snapshot queue logic,
and also shares that logic with the way Inspect works.
The holder's snapshot queue is now provided by the server, in
a default environment.
The queueless snapshot queue no longer triggers snapshots on
enqueue -- it turns out that breaks badly, because a key
point about enqueueing a snapshot is that it's safe to do it
*during* a transaction on that fragment, and triggering a
snapshot during a transaction actually causes horrible errors
as the ops log ends up being the old file, which we close.
Related to this, we also need to prevent closed fragments from
trying to snapshot, so we track fragment openness when opening
or closing, and bail on trying to snapshot a fragment which is closed.
We also stop using the queueless snapshot queue during tests,
because that's a horrible idea.
We copy a little bit of the partition logic from the cluster code so
we don't have to expose it all, this lets us check whether the node
we're looking at is the one which should be primary for a given shard,
and if not, identify which node would be. This works only when
pointed at a data directory, for now.
The test cases for the holder have to be internal, because pilosa
doesn't export view/fragment, just Index/Field. This means that the
holder test cases can't just use the test/* package, so they duplicate
some of its logic, approximately.
This is logically two separate things, but the individual changes
are thoroughly intertwined in the code.
The first change is a logical change to the design of the snapshot
queue, which is that it now adjusts the maxOpN the background scan
targets, allowing it to lower that value over time when things are
quiet. We do this because it turns out that on large data sets,
this can make a factor-of-four difference in memory usage!
So, in general, on a quiet system, each pass through the holder
aims for about 1/4 of the existing fragments to get snapshotted.
When there's more load, we adjust those values up.
We also make the snapshot queue a bit less chatty, to make testing
less annoying -- we only print stats if the queue enqueues at least
two snapshots, or skips any.
The second change is threading the holder through things. We've
always threaded the logger through, and then added the snapshot
queue, and some of the Inspect-related work led to wanting to
have a way to thread options through, so what if we just threaded
the holder itself through, and removed the direct copying around
of the logger, snapshot queue, and so on. Similarly, everything
can now use holder.PartitionN instead of having to get its own
copy of PartitionN handed out to each index.
This does imply ensuring that test cases always get a reasonable
default holder.
This is a precursor to adding additional information to the holder,
such as whether it's in a special read-only mode, which would imply
not modifying on-disk files. This is already semi-supported for
the specific case of the background snapshot queue and cache flushing,
which are attached to the (created in a previous commit) new
holder Activate method, instead of being automatic on holder Open.
The change to a snapshot queue can also cause races in tests, because
the fragment.Clean method's "sanity check" accesses a fragment without
a lock. Fix that. Since there's a couple of t.Fatalf(), but we need
to release the lock before closing, we use an anonymous function
with a defer to handle that. Whee!
There's no reason to have 10-20 seconds of delays for testing this,
because in testing, we're running things on the local machine and don't
need to worry about significant network lag. Make retry count and delay
settable options, and set them lower. Moves the Replica2 test in
server/server_test.go from ~21s to ~2s.
I think this will improve the transaction response messages Kuba
mentioned where it was an empty transaction instead of a nil or not
there... if not it should make it easier to do that anyhow.
also adds a "noSleep" option to the server command to avoid the 5
second sleep we introduced on startup for non-coordinator cluster
nodes. The sleep doesn't seem to be needed in the tests and makes them
much slower.
For tests, we need to create the grpc listener with port 0 in order to
automatically assign a port. This PR moves the lister creation outside
of the grcpServer itself so that we can access that auto-created port.
This PR adds a translationSyncer interface; I tried to include
comments in the code explaining what's going on. This is taken
from those comments:
translationSyncer provides an interface allowing a function
to notify the server that an action has occurred which requires
the translation sync process to be reset. In general, this
includes anything which modifies schema (add/remove index, etc),
or anything that changes the cluster topology (add/remove node).
I originally considered leveraging the broadcaster since that was
already in place and provides similar event messages, but the
broadcaster is really meant for notifiying other nodes, while
this is more akin to an internal message bus. In fact, I think
a future iteration on this may be to make it more generic so
it can act as an internal message bus where one of the messages
being published is "translationSyncReset".
This PR forces the non-coordinator nodes to reset their translation
sync (and therefore their own cosideration of read-only partitions)
any time they receive a `ClusterStatus` message. So basically, as the
cluster grows during the startup process, each node will reset their
translation sync.
This is NOT a good solution log term, but it should address the
immediate problem.
Things to note:
- the coordinator sync isn't getting reset, but that's ok, because the
immediate problem is a partition marked as read-only when it shouldn't
be; i.e. it's ok to have the inverse (a partition not marked as
read-only when it should be) because that partition won't receive
requests anyway.
- the last node to start is already correct and doesn't really need to
reset its sync.
- there are many other scenarios not covered by this fix.
Based on this theory:
```
i have another theory that i’m going to try to test.
this one would only apply in the case where a multi-node cluster is restarted with an existing, keyed index.
- start node0: it thinks it’s responsible for all partitions (nothing is read-only)
- start node1: it thinks it’s responsible for ~1/2 of the partitions and marks the other 1/2 as read-only
- start node2: it thinks it’s responsible for ~1/3 of the partitions and marks the other 2/3 as read-only
now if node0 is the coordinator receiving all translation requests, that still might not explain what’s happening, because in that case it would just do all the translating. i think. but either way, i should make sure that scenario is not happening, but i think it may be.
actually, that might explain it, because what would happen when the coordinator received a translation request, is that it would handle the 1/3 that it owned (now that the cluster is 3 nodes), and it would send the other 2/3 out to the other 2 nodes. but where it sent the requests wouldn’t line up with what the nodes thought they were responsible for based on the restart order
in this example, node 1 would receive requests for the wrong partitions
```
