This migrates existing code from the old TranslateKey(s) endpoints to the newer CreateKeys and FindKeys endpoints.
The CreateKeys and FindKeys endpoints were created previously as the TranslateKeys endpoint had no way to behave sanely when the looked-up key did not exist (the parallel-arrays representation did not have a good way to represent a missing key).
This change also removes the old TranslateKey(s) functions from the translation stores.
It leaves a wrapper emulating the TranslateKey(s) endpoints so that old idk still works for now.
This works around an issue where unreplicated keys will not be matched everywhere.
This also avoids the cost of creating millions of bolt read transactions and allocating strings.
The GetTx logic is deeply broken, this DOES NOT fix the underlying
bug.
When any call anywhere in a given set of calls has a top-level write,
we perform all transactions as write transactions, and we do not cache or
share those transactions. This means that anything which causes a
second GetTx for the same index/shard deadlocks against itself.
The two easy to find cases by casual inspection are time quantums
and Not queries, so this addresses those, but this should NOT be
considered a general fix.
TxBitmap was a workaround for performance problems with doing
individual-bit operations directly on RBF, used only in the
large-writes path of importValue. With importValue no longer
using that path, ever, there are zero remaining users of TxBitmap,
and the test for it no longer exercises it.
Solution: Remove it.
sort.Stable has horrible runtime -- O(n*logn*logn) -- but if we
don't use sort.Stable, our logic for ensuring that we apply the
"last" value for a given column is actually completely wrong in
the first place.
We've got a fairly consistent thing of the API splitting data up
into shards before sending it to a field, which it has to do because
of clustering, so we don't intend to support the case where you
have data from another shard in a data set.
Also drop the identical but mislabeled test from TestIntField's
corresponding case.
There's only ever one view in importValue, but there's also only ever
one shard, because importValue is only called by things called from
the API after it has split everything up by shard.
Since we don't always have "snapshots" anymore, the arguable benefit of
avoiding the snapshot is reduced, and the primary expense of
importPositions has been dramatically reduced as well, so let's
just use that all the time, and simplify life.
We also want to make it faster. We don't know how many bits there
are to set or clear in the input set, but we do know exactly how
many bits there are to set AND clear. We can subdivide these into
batches by rows, then process each batch by storing sets at the
bottom and clears at the top. We can also do batches by columns,
reducing the memory overhead of unpacking all the bits at once.
(For extra credit, we could alternate set/clear settings, and
thus do batches of "the clears from row 0, followed by the clears
from row 1" and "the sets from row 1, followed by the sets from
row 2", and so on, but this is too fancy.)
Every caller of importValue is in fact already providing values
with column IDs sorted. As such, we don't need a map for checking
the previously-set columns; we just need to check against the
previous value.
We also implement, but disable for now, a check for sortedness of
inputs. This check was useful in development but it's expensive (about
5% of CPU time for large inputs!) and once we've verified that we
can make it through tests without triggering it, we're probably fine.
With timestamps, we probably want to at least check larger BSI fields,
so we add that. Also, tweak the interpretation of b.N (making each
N count for 10,000 bits) so we can see allocation load at all. But we
also reduce the sparse set to be about one bit per 19 bits, because
if we do one per 70,000, and are doing field-at-a-time imports, we're
getting hundreds of imports to try to match a target of, say, around
a million values.
We also sort the inputs, because ImportValue is about to start requiring
that, since the API does it anyway.
Also, extend this to be available on Fields, because field.ImportValue
is ALSO doing things which could be inefficient or expensive.
Attributes are unmaintained and unused.
They have become more of a liability than a benefit.
This change eliminates them from the codebase.
The only user-visible change (assuming that attrs are not used) is that the attrs field will no longer appear in row JSON.
When writing things that cause additions to the cache, mark it dirty and
flag it for recomputing, but only sometimes actually do the recalculation,
currently implying a 10-second window. We still mark the cache dirty,
so if a request comes in, we'll get fresh data, but the query will be
slowed down because the recomputation will happen then. But that's better
than doing thousands of recalculations which are never used...
When searching for a small array in a large array, scanning ahead
is productive. The switch from counting indexes to reslicing the
slice appears to improve performance in this case. The fairly arbitrary
value `na << 2` is like `nb / 4 > na` except that it computes faster,
and lets us avoid the expensive overhead unless we have reason to
expect that there's significantly more items in b than in a.
Improvements: Not huge in some cases, but sometimes quite noticeable,
especially as the frequency of overlap increases, which is also
the expensive case in other ways.
name old time/op new time/op delta
ImportMutexSampleData/64K/2Kr/40/none/write-0-8 501ms ± 4% 486ms ± 2% ~ (p=0.052 n=6+5)
ImportMutexSampleData/64K/2Kr/40/none/write-1-8 756ms ± 5% 698ms ± 5% -7.62% (p=0.002 n=6+6)
ImportMutexSampleData/64K/2Kr/80/none/write-0-8 292ms ± 3% 276ms ± 4% -5.46% (p=0.002 n=6+6)
ImportMutexSampleData/64K/2Kr/80/none/write-1-8 511ms ± 6% 482ms ± 4% -5.72% (p=0.015 n=6+6)
ImportMutexSampleData/64K/2Kr/240/none/write-0-8 153ms ± 3% 132ms ± 5% -13.91% (p=0.008 n=5+5)
ImportMutexSampleData/64K/2Kr/240/none/write-1-8 354ms ± 2% 215ms ± 6% -39.41% (p=0.004 n=5+6)
ImportMutexSampleData/1K/2Kr/40/none/write-0-8 565ms ± 3% 543ms ± 3% -3.89% (p=0.015 n=6+6)
ImportMutexSampleData/1K/2Kr/40/none/write-1-8 807ms ± 6% 778ms ± 3% ~ (p=0.180 n=6+6)
ImportMutexSampleData/1K/2Kr/80/none/write-0-8 317ms ± 3% 300ms ± 1% -5.40% (p=0.002 n=6+6)
ImportMutexSampleData/1K/2Kr/80/none/write-1-8 462ms ± 3% 437ms ± 4% -5.31% (p=0.009 n=6+6)
ImportMutexSampleData/1K/2Kr/240/none/write-0-8 141ms ± 1% 119ms ± 2% -15.85% (p=0.004 n=5+6)
ImportMutexSampleData/1K/2Kr/240/none/write-1-8 213ms ± 3% 171ms ± 3% -19.70% (p=0.002 n=6+6)
In BitmapBitmapFilter.ConsiderData, we intersect things solely in order
to perform callbacks on them. Creating these intermediate arrays is
actually somewhat expensive, and all we're going to do with them is
make callbacks anyway.
So, we add a new `intersectCallback`, which behaves similarly to
`intersectionCount`, but which dramatically reduces the amount of memory
allocation associated with doing the callbacks; in some test cases
on mutex data, this code was >90% of all memory allocations, and
getting rid of that helps a lot.
At that point, we no longer need the separate intersectAny check,
because it doesn't save us any time anymore.
The mutex tests had weird and un-idiomatic definitions for b.N, and
in particular would report ludicrously low times for high values of
b.N because they'd still only do a small amount of importing, then
get counted as having done a much larger number of iterations. Also,
the computation of the number of values to create was pretty noticably
wrong so the secondary data set was unduly tiny.
Do tests with ranked cache and larger row counts because we have
reason to suspect that the cache behavior is mattering. We adjust the
range of tests performed to reflect real world data a bit. We also
drop the "don't do large mutex tests" thing because the insanely
bad performance on larger mutex data should be fixed now, we hope.
boltDB's bucket.Put() is quadratic on "new keys put into a bucket during
this transaction", which is why BoltDB has warnings not to use it with over
100k new keys at a time. The translation store logic wasn't actually using
that. The actual value picked is smaller, based on some half-baked benchmarking.
We also avoid heap-allocating separate 16-byte (not 8-byte, of course,
because make(...) is *helping*) chunks twice for each key we insert, instead
allocating a single buffer which we reuse for each new transaction.
Also fixed a check against the nilness of the wrong pointer and generally
made CreateKeys and TranslateKeys a little more similar.
This commit fixes an issue where the root record cache is only
built when a write transaction successfully commits. However, if
no write transactions are occurring then the the cache is never
built and saved so it is recomputed on every read tx.
We check mmap limits, and try to set/increase our open file limits,
and we check the mmap limit when we start the server, and try to set
the open file limit every time we open a holder.
It's useless to do these things more than once, though. We migrate
these things to be run through a sync.Once, which runs all of them
the first time a server starts up, and then thereafter just returns
the error code from that first run. This should make test startup
ever so slightly cheaper, saving us potentially several microseconds,
but also reducing the spamminess of the message.
I've taken out the `sudo ulimit` advice since it's wrong, and the
documentation link is updated to point to our (now private!)
customer documentation.
The "batched" flag creates a complexity which is that the return value of Add
might or might not be meaningful, but it doesn't really buy us very much.
If we are concerned about the ops log size of writing single ops as 21-byte
arrays of 1 op rather than as 13-byte ops, we can make the AddN code smarter
about how it writes ops. And probably should.
Along with this, change Remove to use the batched operation form, which
writes a more meaningful ops log, and return a meaningful value for changes
made. Otherwise, it ends up writing potentially thousands of ops to the
ops log without reporting any OpN, because the number of ops written isn't
the same as the number of changes those ops made. This could result in
files growing by megabytes without OpN changing.
There was a comment here about a test failing with RemoveN. I can't prove
it, but I strongly suspect that this was actually a result of that test
case hitting a particular bug that we eventually fixed, and which we might
have fixed sooner if we'd realized why using RemoveN made that test
fail.
When unmarshalling ops, we weren't adding a meaningful OpN to them,
resulting in misleading reports from `pilosa inspect`. Also, we were
mistakenly reporting things as "mapped" when they were actually
using their internal storage (as with small array containers).
Add the "sanity check" to `pilosa inspect` so that errors like the
above get noticed more easily and corrected. Also, to make that work,
have roaring.InspectBinary actually put containers in the bitmap
it creates rather than just creating info entries for them.
bitmap.BitwiseEqual had a couple of subtle bugs, and the net result
is that if the bitmap you were comparing to had an empty container after
the original bitmap ran out of containers, we'd spuriously report
the container as existing and being... the last container in the original,
actually.
Issues are both that we were grabbing the value from the wrong iterator,
and also that we were iterating twice per loop, and thus could also
have missed a non-empty container immediately following an empty one.
CI systems sometimes hiccup for five seconds, which causes heartbeat leases
to fail and breaks all sorts of things. As a workaround, update heartbeat
TTL for tests only. This might in turn cause different failures to do
with leader elections, but in theory those should be handled now?
A while back we started just polling the reported cluster state of one node
when starting a cluster for tests. This works fine if we're doing fresh
new etcd queries for every single operation -- but that's insanely
expensive, it turns out.
When we use the watcher, some nodes will report stale data for "a
while", where "a while" appears to be easily a couple dozen milliseconds.
This is probably irrelevant in most real-world cases, because the common
case (detecting a node going down) means that we have at least five
seconds after a node goes down before etcd notices the lease expiring,
and a few milliseconds more or less won't matter.
But we have tests that assume either that node 0 is always the
coordinator (wrong) or that waiting for node 0 to think the cluster
is up means that every node in the cluster thinks the cluster is up,
or at least that it means that the coordinator thinks the cluster is
up. We retried later operations but not the initial ones against
the coordinator.
In fact, we probably want to wait for the entire cluster to think
it's up before we start trying things on clusters.
We also replace the "CheckClusterState" function with the existing
AwaitState call, or a new AssertState which errors out since that's
the way we usually use AwaitState anyway.
In the AwaitPrimaryState function, which used to be
AwaitCoordinatorState in a different long-lost revision, we have
to delay until a primary node is available, or fail if one does
not become available, to avoid a panic. This probably shouldn't
happen anymore, because of the last change:
Also, rovide dummy topology.Node entries before metadata is read.
During initial startup, we want to be able to do things like determine
which node is the primary, even before we've read metadata from them.
To do this, we populate the node list with dummy entries that just have
the ID (the only part we need to sort our list), and a node state of
UNKNOWN.
This breaks the fancy logic for determining whether or not to update
the node data, because the initial status of UNKNOWN matches what we
get from SetMetadata giving us new data so we end up not realizing
that this was actually a meaningful change. But actually, that's
a pretty niche optimization; we usually only get state changes when
there's an actual change in state. The updates here are cheap
and only happen after a write (or on the first query) so it's not
worth making the logic a lot fancier to make it work, when we can
just do the simple thing and update any time the dirty flag is set.
We also standardize on a 50ms delay, because 1ms delays were
really expensive when each check was hitting etcd multiple times,
and 50ms is Usually Long Enough.
This is a significant overhaul! Quite a lot of things changed here.
Basically: Prior to this, every request for data from etcd implies
requesting the current live data from etcd, and then unpacking it or
extracting it in some way. This is expensive, which is why we have
a cache in front of it.
We don't need to do that! We can use a Watch, which notifies us
of changes as changes happen. However, there's some challenges and
difficulties along the way, and there's a couple of other changes
which are included here because it's a pain to try to separate them
out.
1. We require a logger to be provided to create our internal Etcd
wrapper. We then use that logger, instead of `fmt.Printf`. This makes
debugging messages work better, and also diagnostics, and so on.
2. The internal client that we are reusing can enter a failed state
after a leader election, in which case we have to recreate the client
to have a working client. We add a new internal-use method,
`retryClient`, which wraps a function which takes an etcd client
and returns an error, and checks for leader-election type errors
and retries creating the client when they happen. That last bit
has not been successfully tested because it's actually really hard
to trigger this now. (Because it was related in part to the
amount of etcd traffic we were producing, which is reduced.)
3. The general swap over from looking things up to unpacking things
as they come in, then returning those already-unpacked things when
we get requests.
With this change, *many tests will fail*. That is addressed by
a separate commit which addresses the secondary problem, which is
that some of our test harness code was relying on the assumption
that if any node in a cluster thinks the cluster is up, every node
will. That was usually true when we were doing everything as
expensive fully-synchronized cluster checks, but becomes significantly
less reliably true in real-world cases where nodes are also
going down sometimes, or nodes are going up and down unexpectedly.
The new etcd implementation has internal caching-like behavior which is
much more reliable (it doesn't use a TTL, it just updates when there's updates
to process) so we don't need this cache.
The existence field wasn't working because runs were broken for filters in
RBF. Fixing that allows us to simplify the logic. Also, we reuse the
findExisting filter because the filter's cached collection of containers
can be reused between things, allowing us to reduce allocations when
there's a lot of views.
The remake container logic (used to avoid allocating extra containers while
applying filters) relied on roaring recomputing N, which it did for bitmaps
but didn't do for runs. Fix this both ways; it would now do that for runs,
but also we add "with explicit N" variants and use those since we have a
correct count already, and don't need it. This means fewer popcounts on
bitmaps, and working at all on runs.
This is used to handle a possible case where a kafka partition is moved to another ingester while a previous ingester is still processing it, causing 2 ingesters to process it at the same time.
This allows a duplicate ingester to skip past messages which have already been ingested.
Due to lack of synchronization, this test would sometimes close the DB before terminating a transaction:
=== RUN TestTx_CommitRollback/SingleWriter
tx_test.go:132: db still has 1 active transactions; must closed before closing db
The test now waits for the goroutines to terminate before closing the DB.
Before this change, we were only caching the BitDepth on the
field.options. This was ok as long as applyOptions() was called after
that. But unfortunately, during startup, applyOptions() was called prior
to that being set. So with this commit, we explicitly set the value in
bsiGroup.BitDepth as well.
The ID allocation API was broken because the operations were removed from the list allowed in the NORMAL cluster state.
Additionally the operations were set to only run on non-primaries (where they were actually only supposed to run on the primary).
There's some loose ends here because really we probably want to be
using the top-level server logger, and we should fix that, but in the
mean time, let's not swallow the errors as much, because the last
line printed doesn't actually show what the error was, but it could.
To do this, we distinguish between the current error (which might
be a wrapper around DeadlineExceeded) and a previous error which
we might prefer to return, if one exists, since it's more likely
the "real" cause.
In nearly all cases, we can just switch ioutil.TempDir->testhook.TempDir
and similarly for TempFile. There's one case where we can't because we
need files to be removed before tests are over.
Also in the process give identifiable names to a lot of temporary files
and make sure they're being cleaned up, and don't use "/tmp/foo" as a
file name in a test that could be running in more than one test process
at once. :)
Prior to using etcd for node membership, the data director was created
during the cluster topology setup. Since that no longer exists, we
weren't actually creating the data directory before getting to
logStartup(). So this change ensure that the data directory exists.
There's no need to have two different translation readers, a single
reader can handle both partitions and fields at the same time, so we
can combine them. This may not actually change things much but was
a useful step in diagnosing a different problem with translate readers,
and I think it is a minor improvement so I'm preserving the patch
just in case.
The functional option and returned closure combine to result in
us using the same sync.Mutex object for every TranslateReader on
a given server, which means that if one of them isn't producing anything,
we eventually end up waiting on that with all the others blocked
waiting for the lock. Use separate locks for each, of the same
type as the one initially provided as a template. This does mean
that multiple readers can be operating at once, but in theory
no two readers should ever be writing to the same stores, we
think.
If we are using replication, we can be a replica translate store for a
partition, which means we start a translate store reader to replicate
data for it. The translation logic does not admit *stopping* the
translate reader, only "resetting" it (stopping and immediately
restarting), so the translate reader just runs until it hits an error
and terminates, which it does even if perhaps it shouldn't. Oops.
Anyway, one potential failure mode is that if you hit timing just
right, you can end up trying to process translation *while* the
index is being closed, and the index can close its translation stores,
and make them all nil, right before we request a store and try to use
it. Another is a similar error, but during the initial startup of the
translate store readers. Either way, we want to error out of the
process cleanly if this happens.
This could also happen during initial creation, perhaps.
We're aborting translation sync on these errors, because otherwise
we'd continue accepting new keys, and then end up with our highest
known key being higher than some keys we missed; this way the next
restart will restart from the last key we have.
this doesn't quite work as-is, but I verified that it reproduced/fixed
the issue by adding a panic where the problem log statement
is. There's a follow up ticket to fix the test... it's just a bit open
ended as to the best way to do that.
we were sending pilosa.Message objects from a spool, but actually
passing a pointer to them rather than the Message itself. I'm
concerned this wasn't caught in any test, and also curious if that
needed to be a pointer for some reason or if it's a typo.
Definitely need to write a test still.
Ensure that mapReduce always waits on its ErrGroup, even if it wants to return
early due to a failure somewhere. Also check logic a bit more carefully on
the error returns; we don't want a transient failure from one node to result
in the whole query failing, we just want it to retry on the next node, so that
shouldn't cancel the whole ErrGroup.
if we got an error, we don't have to merge it. so either ctx.Err or
resp.err being non-nil means we shouldn't be reducing, but we still need
to grab the responses to make sure we waited for them all.
This fixes a variety of bugs where API requests would read uninitialized state, causing crashes or race conditions.
Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com>
It's not enough to cancel jobs so their goroutines *will* exit; we have
to be certain that they *have exited* before we finish returning from,
e.g., mapReduce(), or a query can "complete" at a time when there are
still running goroutines accessing data that we're about to invalidate
when we terminate the Qcx.
A better solution would integrate this logic and control into the Qcx
and pass it through everything, rather than having the Qcx bypass
the mapper/mapperLocal and be passed into the mapFn/reduceFn via
closures. But a better solution would be a lot larger.
This gives more consistency with the other tests and allows us to get audit
checks on the server/ tests. The tests on the clients being closed are
temporarily disabled because they tend to think the last test's clients
are "still open" for a few seconds after the test completes.
etcd runs a LOT more goroutines during server startup. Fix a
goroutine/for loop bug causing us to run four 7-node clusters
instead of 1/3/4/7-node clusters, also have the test/cluster
code reduce import workers. We can't do much about the spamminess
of the Raft stuff, but this should tone it down some.
Every usage of this just ran keepAlive func as a goroutine with a timer, using
a parent context, but the keepAlive func didn't know about that context, so
it couldn't use that context for its own messages or interactions. Change
it to create its own cancelable context from a provided parent, and use
that to control its inner behavior.
Note that we *do* still need to send the revoke at least sometimes -- otherwise
cluster states don't update correctly. But we can time that send out
rather than using context.Background(), because after a TTL's worth of time,
there's no lease to revoke anyway.
Also, add hooks for testhook tracking so we can confirm/deny that things
are getting shut down, which they weren't.
Long story short: Once we create a server and start it, we can't start
it again. We can't close it and restart it, and we can't just start
it without closing it.
Unfortunately, if the server's config needs to change, we have a Problem
here.
This ultimately means that the retry logic for GetListeners can't actually
retry successfully; if we fail on the first attempt, we necessarily fail
on any later attempts also, and if we try to fix that, we get panics.
But!
We don't actually NEED to retry. We just need to ensure that we can
open a :0 port, extract the actual port number, and use that in places
where the port number mattered, without having to rebind it.
The only actual place we needed to rebind things was opening gRPC
servers, so we introduce a gRPC Listener that can be used instead of
trying to bind to a specified port.
In a bunch of other cases where we had similar logic to try to allocate
and then use a port, we can switch to just using a provided listener.
For instance, net/http has `Serve(net.Listener, handler)`, not just
ListenAndServe(addr, handler).
This should eliminate the weird CI failures from eaddrinuse.
NOT fixed: server/cluster_test.go/TestClusterResize_AddNode isn't working
right now. The new node isn't actually being added to the existing cluster.
I attempted this but was outsmarted by it, and I think fixing the
rest of this is worth it as a separate thing.
Currently, if you issue a node removal from the node that is being
removed, then you will see a "node cannot be removed error". It's
not clear why you aren't able to remove the node. The error message
has been updated to clarify why.
There is another case where cancelling here might be useful,
and that's if the query is on a primary node and the replication
factor is 1, meaning there are no secondary nodes to fail over to.
That case is handled here as well.
Turns out we sometimes modify returned nodes. Handle this better, but
also fix up some cases where we were generating node lists we didn't really
need to answer simple questions.
If we're the primary field translation node, we don't need to set up
translation replication; we only need that if we're *not*. So it
makes sense to test if !IsPrimaryFieldTranslationNode... except that
the test is to determine whether to return early. So it should not
be inverted.
change logic in version.go so that the trial related messages only appear on trial versions of molecula
convert Command methods in trial.go to functions and pass a loggerLogger variable instead since that was the only piece of Command being used
add a function named expireAfter which seperately runs similar functionality to what was previously in daily check with chnages directed at stopping users from changing their internal clock date
change variable names and placement to be more readable and organized
change logic in version.go so that the trial related messages only appear on trial versions of molecula
convert Command methods in trial.go to functions and pass a loggerLogger variable instead since that was the only piece of Command being used
add a function named expireAfter which seperately runs similar functionality to what was previously in daily check with chnages directed at stopping users from changing their internal clock date
change variable names and placement to be more readable and organized
When *fragment.openStorage is invoked in both f.importValue and
f.importValueSmallWrite and it returns an error, this means there's
some underlying error with the storage device and at the point of this
commit, the sane thing to do is to close the process, otherwise the
operation of Pilosa might proceed in an inconsistent state thus
precipiatting other silent but hairy errors along the way such as
dereferencing *fragment.gen later on which is set to nil once
openStorage fails.
This changes Count(Precall()) operations to execute the precall directly inside of the count operation, bypassing the transformation to a Precomputed() call.
Eliminating the Precomputed() step causes Count(Distinct()) to work properly on negative integers.
We need to be able to count bits in BitmapPtr containers. This only
comes up if you have a non-container-aligned range count, which we
never do in real production yet, but the API allows it so it should
work. In order to do this, we need to provide the tx to countRange
so it can grab pages as needed. Arguably, we should be able to avoid
actually creating/copying that page since we're only using it
internally, never returning it, but this is a pretty rare case
and probably not performance-critical.
This test was supposed to check against all the container types,
but especially bitmaps, but turns out not to work because the
containers turn into RLE containers. Oops. Now, we start with
every-other-bit for the first 8k, then start filling in the holes,
so we get some bitmap containers and then start generating
RLE containers.
this commit adds a temporation interface for starting gossip.
we needed this so we can start gossip AFTER setting up the node,
but before waitingForJoins.
When a run started and ended within a single word, the entirety of the word would be checked.
This would cause small runs to be processed incorrectly, and caused Distinct-on-sets to select rows that did not match the specified filter.
We want to distinguish different *kinds* of GroupCounts, so we're
making the GroupCounts parent object track its type so we can keep that
correct.
Adding this to protobuf, etc, then creates some weird behaviors
because sometimes we expect []GroupCount, and sometimes we expect
*GroupCounts. This implies changes to test cases. Also, the
changes to test cases imply that some test cases are probably now
wrong; for instance, they're expecting a "sum" column, equal to zero,
when no sum was requested.
We try to make the encoder handle a []*GroupCount gotten from another
node without panicing, and avoid breaking the semantics of the existing
messages, renumbering messages or components, etc.
Since a previous version, the `.Groups` member has been privatized,
and the `.Get()` convenience accessor has been renamed `.Groups()`
and is now used consistently in a way that should reduce the risk
of nil pointers causing crashes. Also, NewGroupCounts is used in
a couple more places.
port mapper gives out ports from 63000-65000 for the tests
fix another race
http test uses port.MustGetPort
rbf: remove :0 port request
ocd happy
test fix for grpc listener address already in use
test/disco allocates BindGRPC port from the port mapper
dump stack on each GetPort
verify each port is usable right away
server/config.go has Config.Validate() now
panic if gossip port is 0. validate server.Config
fix another gossip port 0
builds
quiet, don't dump stack on each port alloc
builds
happy linter
even gossip fallback should not be zero but rather use the port mapper
If a precomputed call returns a nil Row result somehow, that could
cause a nil pointer exception when handling the result in
handlePreCall.
In this particular case, A Distinct call on a BSI field with a filter
which returned no results could return a SignedRow{} with nil *Rows
inside of it. This only manifested if there was data in a single shard
as otherwise the reduce logic created a SignedRow with empty *Row
objects rather than nil ones. Isn't that fun?
Extra fun: the reason the filter was returning no results was not
because it was actually empty, but because of another bug where
constructing the Distinct calls to compute the aggregate of a GroupBy
doesn't take into account that the group might include an integer
field which means that the call needs to be constructed
differently. That bug is not fixed in this commit, hence the tests are
still failing, but not panicking.
Add ability to sort on count or aggregate in GroupBy. Fix bug with offset being unsupported. Fix bugs with limit interacting poorly with other arguments.
the limit could get applied before "having" in some cases which could
result in results being discarded which met the having condition while
results were kept which did not, ultimately resulting in GroupBy
falsely reporting fewer results than actually existed.
Back out support for sorting on fields (only count and aggregate
supported for now).
Fix bug where default return of "true" caused sort to be unstable. (If
they are equal, Less should return false)
Fix bug where limit was being applied before sorting.
Fix bug where offset was not actually allowed to be an argument to
GroupBy (weird! guess we weren't testing that very well)
Apply "having" after calculating Count(Distinct) aggregate so that
having can apply to that.
Switch to stable sort to make testing easier.
We execute the aggregate Distinct calls after the GroupBy is complete,
and we need these to act like non-remote calls in that they forward to
all nodes, but like remote calls in that they bypass key
translation. Added a "PreTranslated" flag to the QueryRequest to
achieve this.
Discovered an issue where a nil *Row in EmbeddedData would cause a
panic in the protobuf serialization. Changed the encoding code we
control to never pass a nil *Row.
Got fed up with lack of context on errors and added wrapping to all
calls under executor.executeCall as well as a few other places.
Handled a situation where not having data on a shard for a particular
field could cause a query to error instead of just treating that
fragment as being empty. (see the switch in executeDistinctShardSet)
Stopped GroupBy from executing the Count(Distinct) aggregate on Remote
calls.
Fixed a longstanding issue where errors retrieved from remote query
calls had a garbage character at the front due to treating a protobuf
payload as an error message instead of decoding it. (see
http/client.go)
If you're running a Pilosa with mostly default configuration on your
system, some of these tests would fail due to things like port
conflicts. These changes address the most common failures.
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.
This commit changes executeDistinct to return either a *Row or a
SignedRow (instead of only being able to return a SignedRow). Distinct
on a set field will return a *Row while an int field will still return
a signed row.
We then add Field and Index fields to the Row object so that we can
determine how to translate the rows IDs to keys (if needed). This adds
a lot of logic around the translation which fixes bugs where Distinct
calls would fail to get translated.
There are, I think, still issues if you were to try to join a keyed
field to a keyed index which wasn't explicitly specified as the
field's foreign index. The IDs in the field wouldn't be using the same
translation as the IDs in the index, so the query might appear to work
but give incorrect results.
add Distinct test with integer data, and because one of the records
had a null value (and was in a shard by itself), it uncovered this
issue. I added a special error type if a view or fragment is not found
when so that we can match against it and ignore it when calculating
the results for a query.
I also added an implementation within executeCount to handle the
SignedRow case, but discovered that handlePrecalls always dumps the
negative data and that will be a bigger thing to fix
the Distinct call would get precomputed correctly, but then the
executeCount would happen in the available shards context of the
index. So if the index only had records in (e.g.) shards 10,12,18,22,
and all the values of the Distinct call were in shard 0, you'd see 0
results.
The fix skips the whole map/reduce step of executeCount (which was
basically fake anyway when the argument is precomputed), and just adds
up all counts of all the precomputed segments.
This currently won't properly count Distinct values from an int field
which contains negative numbers... going to add a test and fix for
that next.
There is also still a key translation bug which is why the one test
case is commented out... fix coming for that soon as well.
- Previously, on timequantum schemas, we would
create and open a view for the cartesian
product of every possible view and shard.
- This caused us to be very slow on re-open,
and to use lots of memory for views that
held nothing.
- This change makes startup faster, memory
use much lower, and should speed migration.
this should avoid a race condition with CreateField where createdAt
can get out of sync if there are multiple concurrent requests.
The client methods didn't allow specification of the URI, so I
modified the implementation to find the coordinator and send to it
explicitly.
unclearSets was completely broken and I have no idea why the test I thought
was testing it didn't actually catch that problem. Added unit tests and fixed
the logic. Improved/clarified prune and fullPrune, and unexported their
names because why export methods on an unexported type.
Also improve some comments and rename a variable or two to improve clarity.
On roaring, CountRange needs to have exclusive access to a fragment, but
doesn't currently require a lock, because it's usually used from inside
other already-locked things.
CountRange for RBF had a subtle bug which wasn't noticed, so, let's
have some CountRange testing and also a benchmark.
We also fix a couple of subtle bugs caught in the process of developing
and testing this.
SliceContainers will allow nil containers, but doesn't return them when
iterating because there's various things that can panic if called on a nil
container. Since countEmptyContainers() has to traverse the whole bitmap
anyway, it doesn't matter which it counts, so we replace it with
countNonEmptyContainers(), and adjust test cases accordingly. This fixes
an issue where if roaring is smart enough to insert a nil container
into a SliceContainers, trying to write it to a file produces an invalid
bitmap with offsets off by 16 and one container fewer than its header predicts.
RBF: don't try to count 0 bits in a container
If we're to the "last container", and we'd be counting all the bits less than
zero, we can skip that. This avoids hitting a bug, which is that c.countRange
doesn't handle BitmapPtr.
Several issues:
1. tx.frag could be non-nil but not the fragment requested.
2. start and end need not be exact row boundaries.
3. therefore this could be returning the count of the row containing
"start", for a fragment other than the one requested.
4. also in fact the rowcache wasn't populated before this so in one
memory profile, this function alone was responsible for nearly
100GB of cached values...
In some cases, ApplyFilter can be significantly faster. On the other hand, it doesn't
matter as much as you might think on the mutex imports, because we've already sucked
most of the time out of those.
Added additional mutex sample data and batches of it so we can
confirm that overwrite works. It didn't work, so that needed to be fixed.
Couple of things:
(1) Wasn't updating "last value seen" so the check for an unsorted list
didn't work.
(2) Also didn't handle the case where there were to-clear values higher
than any to-set value.
This could result in bits not getting cleared, which could result in
there being more than N bits to clear for N new bits. And that could cause
really strange problems when the input slices were parts of a single
larger slice, because bit positions to clear could get shoved in as
possible columns in a future batch.
For the Rows benchmark, we were continuing to use the original writable
transaction, meaning RBF was spending all its time looking up dirty
pages in the transaction's dirty page cache rather than working with
the disk in any way. It wasn't clear whether this was hurting or
helping performance, but it was clear that it wasn't testing the
"real" workload use case, where queries are done against the RBF
file rather than the dirty page cache.
Modify the benchmark to test it both ways for comparison. Answer:
The RBF file is faster than the in-memory map (!).
This reduces noticably the cost of reading leaf cells, by passing
a single pointer down the stack instead of the entire data structure
up the stack. It's only a few percent overall, but it's noticeable.
This gives RBF an ApplyFilter that can run without instantiating containers
when the filter it's using doesn't need them instantiated. We can also seek
ahead in cases where we know the next key we care about is not just the next
key numerically.
This is a partial solution to a nasty performance problem, which is that
a ContainerIterator has to *generate* all the containers. With roaring, this
was cheap because they already exist in memory; with transactional backends,
it's an allocation per container, *even for the containers we don't use*.
This design admits filters which can distinguish between answers they
can give just based on keys and times when they actually need containers
instantiated, and can also give hints as to future answers -- saying "yes"
or "no" to entire rows at a time, or indicating when they're done.
This is only part of the solution; we also need a Tx API hook for
doing scans like this which doesn't rely on ContainerIterator.
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.
- use short_txkey for rbf
- short_txkey breaks a bunch of bolt_test.go, so leave it on (long) txkey for now.
- remove SliceOfShards method from Tx interface
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
This adds a "duration" parameter to RowResponse and TableResponse, which
will be populated with the query duration in nanoseconds.
For QueryPQLUnary and QuerySQLUnary, the duration is a included in the
returned TableResponse.
For QuerySQL and QueryPQL, only the first RowResponse in the stream will
contain the duration.
The RandomQueryConfig has its own seeded RNG, but we didn't always use
it (especially in the last two commits, but also I think in one previous
thing), so let's use it more consistently.
We check for int fields (as opposed to decimal), and if we find them,
we add Distinct to our list of potential queries to use if and only
if we've got a depth of at least one so there'd be a child query under
the current query, and if we do, grab one of the int (not decimal)
fields and do a query on that.
For time fields, allow specifying a range of times, then 19/20 times,
specify "from" and "to" times in that range when querying those fields,
rather than just looking at the standard view all the time.
More generally, report QPS not at 0 queries, which is boring, but every
100 queries *and* after the last query if the last query wasn't at a
multiple of 100 queries. Makes the output slightly more useful, I think.
This replaces the former TopK BSI building algorithm, as the row cache was too expensive.
Additionally, BSI addition has been optimized with specialized adders inside of roaring.
- the sync.Pool default uses little memory under CI.
- arena approach provides ability to control the maximum memory
used by rbf Cursors.
- cursor caching is adjustable with --rbf-cursor-cache
currently 0 by default (meaning use sync.Pool), and
larger than 0 meaning use an arena of this size.
With the arena, 20 or less is needed to pass CI.
- rbf test suite runs ~ 4x faster
- kitchen sink ingest test runs 16% faster.
- report TotalAlloc in CALLSTATs
fixes#1105
This commit fixes a bug where the root record cache was being
updated in-place causing a race condition with other transactions
using it. The cache implementation has been changed from `rbtree`
to an `immutable.SortedMap`.
- fix a bug in computing leafCell.BitN in a run after a bit Remove
- shrink bitmaps on remove
- util_test.go has Cursor.DebugSlowCheckAllPages to verify;
used by cursor_test.go
- default Tx is once again RBF, changed from bolt.
- document the RBF code review comments that were not addressed
before #1052 was merged, so they don't get lost.
- they should be easily addressed by replaying the entire WAL file
rather than from the DB meta page 0 notion of the last WalID
- cleanup rbf/cfg/cfg.go stale comments, ensure default0 respected.
1 msec checkpoint time, 1MB wal segment defaults.
- return a specific error, ErrNoMetaFound, from findNextWALMetaPage()
rather than io.EOF, since there actually wasn't any file IO involved.
- add http handlers for /cpu-profile/start and /cpu-profile/stop
in http/handler.go enable CPU profiling at specific time points
during an ingest or other operation.
- to indicate that the query context is already
done.
- handles the case where the import worker is
interrupted early by a ctx cancellation,
thus avoiding a panic.
This commit changes the checkpointing to determine a minimum WAL ID
for readers and a max ID based on the writer. Pages are checkpointed
from the WAL up to the writer's max WAL ID but segments are removed
only up to the reader's minimum WAL ID. This ensures that WAL pages
are not removed out from under current read transactions.
Our nightly CI has been failing for a week due to WAL issues and it's
making it difficult for Kuba and Antonio to do things on the
integration repo. Hoping bolt backend will solve that in the short
term. I think the issue is Pilosa #1046 (that's from memory though)
Per slack discussion with Seebs and Nia,
we'll try not automatically resetting the Qcx.
The worry was that our goroutine shutdown
management is so poor that we are asking for
GetTx on a goroutine that still has a Qcx
from a query that was cancelled.
If this is the case, we will now panic instead of
issuing a new Tx. Then we can fix the poor
goroutine management.
- also require Qcx.Finish or Abort before Reset
- only allocate the rowcache if it is in use (avoid allocation per fragment)
- when the rowcache is use, fragment.go intRowIterator must write lock the
fragment because the f.rowCache will be updated.
- eliminate unused bitmapCache interface to keep the linter happy.
- fixes#1035
- we return to checkpointing after every commit, by default.
- the internal rbf logic is not ready to have
checkpoints deferred. Doing so results in
references to WAL segments that are not
in the current slice of live segments.
- 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 "seenThisRow" value was never getting cleared, which meant that
if the first container on a row didn't happen to contain any post-filter
bits, the rest of the row wouldn't get evaluated.
Add an exported IntersectionAny() from roaring to let us quickly
check whether two containers have overlap, so we can avoid performing
intersections we don't need to when evaluating containers within
the same row as a previous match. (IntersectionCount on the whole
bitmap would imply doing up to 16 intersections even if we find a bit
right away.)
We also allow ForeignIndex to be set on set, mutex, and time fields,
since all of those could now be reasonable operands for Distinct
ops.
Not yet present: Handling time quantums, but that seems really
desireable.
This adds a series of archetypal containers that represent the
common use cases (arrays, bitmaps, or runs of various cardinalities)
and runs the basic operations against them for benchmarking purposes.
`benchpretty` is an app to snatch the BenchmarkCt* lines from
benchmark runs and display them in a possibly more usable form,
mostly as a precursor to cool analysis things.
This also adds a test to verify that intersectionCount(a, b)
is the same as intersect(a, b).N() for all the archetypal
containers.
This also includes a performance fix for intersectBitmapRun which
was spotted while running these tests.
Go killed off using the common name for hostnames starting with 1.15,
but this can be addressed by recreating the certs using a Subject
Alternative Name for the domain for "localhost". This allows tests
to pass without hanging, at least for me.
- fix a CI/Makefile issue that was hiding red tests in CI.
- the testv and testv-race targets now require /bin/bash
- In executor.go, the top-level query context Qcx now
has a write flag. It will upgrade read-Tx to write-Tx
when Store() wraps some inner local-read operations,
to avoid deadlocking against its own query. This deadlock
happens in TestExecutor_Execute_SetRow/Set_NewRow
under rbf_lmdb blue-green testing without the upgrade.
- correct string constants for txtype so that
blue-green cleanup correctly detects when
2nd transaction in a pair has Committed and
thus the blue-green RWMutex can be relased
- test that txtype.String() is consistent with
the corresponding string constants.
- document in bluegreentx.go the current limitations
of blue-green testing: only one github archive import
(a single writing client) is supported by blue-green
testing. Multiple importers will deadlock eventually
on the DBShard.mut RWMutex. We could fix this by
ordering the write locks and obtaining them in
strictly increasing order (by shard number), but
that would require alot of change to the executor
and that would introduce more risk for a test-only
pathway.
- allows blue-green testing with concurrent readers/writers.
- otherwise we don't start/end the blue and green Tx
together, and they get split by a read/write concurrently.
I have a newer staticcheck and golangci-lint on my laptop, and it started
complaining about something. The first comment added disables the check
in staticcheck-as-a-command, the second disables it when it's being done
by golangci-lint, which invokes the analysis passes directly and displays
the output differently, and also doesn't recognize the hints used by
staticcheck.
Newer golangci-lint doesn't find anything else that it wants to complain
about.
The updated peg tool produces an Init that can return an error. As
of this writing, the error can't be non-nil unless you specified an
option which itself returned an error, but that could change later,
so let's be careful.
There are subtle inconsistencies, like "01" being a valid decimal but not
a valid integer, which vaguely bug me. Cleaning this up, and the corresponding
parser logic.
A number can't have leading spaces because the grammar doesn't
put spaces in them in the first place, so stop accepting them in the
number syntax. This should never have any impact on anything,
it's just simpler.
Update a couple of test cases to reflect this -- no longer testing
that trailing spaces are okay, now testing that they're not, for
instance.
We still prohibit a space before a leading '(', which maybe we shouldn't,
but we now allow spaces on both sides of a closing ')' more consistently.
Drop the unneeded "sp" before "close" in the special handling after
null, true, and false, because close now implies that.
Also, refactored the two instances of "sp '=' sp" into a thing called eq,
which may not be worth it.
Use "" strings for fixed string names. In startCall(), look up the
lowercase conversion of a call name in a table mapping all-lowercase
representations to canonical case, so we don't have to chase down
everyplace in the rest of the code base that assumes "Row" is
capitalized exactly like that.
PQL always produces decimals, which have effectively-arbitrary range,
but can convert them to floats when required; the executor then requests
this conversion in the handful of cases (SetRowAttrs and SetColumnAttrs)
where it wants floats rather than decimals.
Not yet fixed: The "Range" call may also be wrong now. It was specifying
an "fvalue" but is now effectively getting what used to be called a
"dvalue". However, so far as I can tell, that didn't work before either.
Drop irrelevant (), simplify the expression of the sp rule.
Perhaps shockingly, this *does not change the generated grammar at all*. The
generated code for:
sp <- [ \t\n]*
and is identical to the code for:
sp <- ( ' ' / '\t' / '\n' )*
And in fact, is spelled the latter way in the generated comments.
Want to do some PQL cleanup. A new version of peg turns out to dramatically
alter performance in some cases, so I'm doing the commit for "don't change any
PQL, just change the version of peg" checkin separately.
1.14 had a bug in the checkptr code (well, not exactly a bug) which
made it enforce alignment requirements on x86. This turns out not
to be the problem I was seeing, but we should be on 1.14.9 anyway.
To keep this from breaking CI integration with Github, we also
use explicit job names instead of matrix-generated ones, and fix
the CI config syntax up a bit after almost getting that right the
first try. (This patch includes fixes contributed by Cody, and since
I had to rebase and re-approve AGAIN anyway, I might as well squash
the commit history up.)
Step one: switch to etcd.io's bbolt fork of boltdb.
The etcd-io fork of boltdb isn't archived, and has fixes for boltdb's
interactions with checkptr, allowing us to drop the checkptr-disabling
hackery.
This seems to be a drop-in replacement; etcd/bbolt says that the file
format is "fixed" (I believe in the sense of "unchanging"), and I can
run pilosa on an existing data directory with this.
Step two:
Fix missing caps in roaring.go that were also triggering the same
issues.
Add test of single = int query over multiple shards which reproduces
the race
move the code which modifies the PQL call object if a Row query
on an int field uses a single = instead of ==. Instead of processing
this at the shard level, we'll process it during the initial
translation step so that it isn't operated on concurrently.
In rare cases, RBF can produce containers which have a recorded N value which
is incorrect. This rarely affects anything, but on some particular queries,
this can result in very strange outcomes, like array containers with more
than 1<<16 entries.
To fix this, we have toContainer specify that it doesn't know the correct
N for the bitmap containers it's creating, which costs extra time for counting,
and should be considered a temporary workaround.
Also, we add a CheckN() function which is controlled by the
roaringparanoia flag, and add a number of calls to it, for instance, as
deferred calls after every container operation when roaringparanoia is
enabled. This means that we get improved confidence that we've caught
the relevant errors, but is not suitable for production use.
Previously, RBF shared a list of WAL segments between the DB & Tx.
However, this increased the need for mutexes to access the data.
WAL segments are effectively immutable on-disk so the list of segments
has been refactored so that changes to the segment list are done via
copy-on-write which allows read transactions to access segment data
without a mutex.
The database checkpointing can remove early, unused segments and
there is an update/add check to make sure that Tx segments pushed back
to the DB do not include removed segments.
- 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
At some point the cluster code was modified to do 120 tries to confirm
if a node was down which is a bit excessive for production. My
understanding is that this was done to help trigger or fix a problem
during testing which is hopefully no longer relevant.
Also changed the tournament script to use what I think are more
standard bash-isms that work on mac. Please confirm this still works
on Linux as well.
The logic assumes that the lack of a corresponding rowSegment means that
there's no changes, but that's not true -- we just deleted all the
existing data! Update to match clearRow behavior better.
Also, add a corresponding test case for this.
Also, change references to 'defaultSnapshotQueue' to use
[fragment].holder.SnapshotQueue, because defaultSnapshotQueue was
the queueless queue, but holders were getting a snapshot queue,
meaning that "awaiting" a snapshot could result in moving on
and closing the holder before the actual snapshot queue finished
snapshotting.
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.
The copy-on-write/rowCache changes require that functions that
modify containers be able to generate new containers. Once that
became possible, some significant pool of other operations
started relying on it -- for instance, operations might return
a new container even though they're in theory "in place" operations.
I developed a tool for checking for unused function return
values (github.com/molecula/noticeme), and ran it on this, and
picked out the places where `*Container` values were generated
but not used, and some of them seem to be potentially-real
bugs, and a few are probably harmless. Updated code to make
those diagnostics go away.
This is no longer necessary, as caches now recalculate on read.
Also, in general a user will not explicitly request recalculation, so it would make sense for our tests to reflect that.
- log Debugf when we repair a fragment block
- better run-run roaring testing for over-sized containers
- add which fragment path to panic on container too big
- include container contents in roaring hash for pilosa-chk/pilosa-check-backup
This commit fixes an issue where direct writes would overwrite the
source page where data was being copied from because writes are
immediate (instead of going to the WAL first).
- on startup in blue_green mode, we will migrate
blue to green if blue is empty.
- otherwise, when blue has data, we verify
against green before proceeding with the
blue_green run.
- small optimization in the rbf cursorx.go to
short-circuit processing on a nil bitmap.
This avoids a roaringparanoia tag panic.
- back out holdbkg.go, was too slow.
add a distinct Holder.imu lock instead.
This adds testing for Store(Distinct(...)) with and without filters, to verify that
we can, in fact, store the results of a Distinct() query directly. This was at one
point unsupported, now we think it should work so we're testing it.
The change to the testdata is because the specific structure used for this test doesn't
work with a keyed index, and changing things to be "foreign keys" seems annoying and
more complicated, but possibly that should become part of a future test.
There was talk of testing this with non-BSI fields, but they don't seem to
actually work with Distinct right now, so that will be later.
Thaw() is supposed to always provide writable storage, which it does
by ensuring that containers aren't frozen, but also by cloning or
copying their data if the data is marked as being memory-mapped.
But only the roaring backend had the ability to mark data as memory-mapped,
because that wasn't exported. Fixed this, and added corresponding code
to badger, lmdb, and rbf.
- add tournament.sh to do all pair-wise comparisons of blue-green backends.
- isolate txstores away from roaring index/ directories with indexname.index.txstores@@@ dirs.
- blue_green for doing migration. Called before Holder.Open finishes.
- holdbkg.go added for index lookup. Less wedging between a deadlock and a race.
- fix fault under read-only map under lmdb at
TestExecutor_Execute_Row_Range/RowIDColumnID by doing cow in roaring.
- roaring -tags gofuzz builds again
- roaringparanoia build tag added to make test targets in Makefile
- add rbf.NewDBWithAllocZero for out-of-bounds memory checks
- .circleci/config.yml test-shardwidth-22 with large run container, kept OOM-ing we suspect.
Fixes#819
After documenting the semantics, I noticed an arguable hole in them,
which is that you could Freeze() a dirty container, and then Repair()
wouldn't work on it. On further study, I added a roaringparanoia
check for attempts to access the N of dirty containers.
It turns out there's several such. But also, it turns out, there's
circumstances where unionInPlace is relying on the assumption that
N is valid, which it isn't always for dirty containers. Also, there's
at least one case where we rely on the assumption that forcibly
thawing a container, then calling unionInPlace on it, always modifies
that container. But that's not supposed to be true for an empty
container -- an empty container might be better handled by just
returning the container it's being unioned with. So, we drop the
unnecessary thaw (all the *InPlace ops are already thawing if/when
they need to), but we use the return from unionInPlace.
If you just stash the results of the function when defining the test cases, the
outcome is in part that you are reusing the same slices for multiple things. So,
for instance, if you perform a union on the OddBitsSet slice, with the EvenBitsSet
slice, the result is to overwrite the first entry in that slice with the 0-ffff
run... But the original slice still exists, and then we reuse it and get a slice
with a bit count of around 98,000. The underlying issue is that doContainer()
is calling NewContainerRun(), which is simply using the provided slice, not
copying it -- which is intentional, but the test has to be careful about it.
We call repair on the one we think should be a bitmap. Theoretically
maybe we should also repair the other one in case unionRunRun some day
starts returning unrepaired bitmaps, which in principle it's allowed to
do...
The copy-on-write semantics were previously documented only in
the 125-line commit log from the patch which introduced them. Add
documentation for them in a few likely places.
blake3 code is used in several places on the code. The file was
duplicated on root and rbf package.
To avoid cyclic dependencies, I moved it to hash package. Some methods
must be public to use them in different places.
HashOfDir method was removed. Not used.
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
This commit adds the ability to start a transaction with an exclusive
lock for the entire database. This ensures no other read or write
transactions can run at the same time. Writes in this mode write
directly to the database and skip the WAL entirely.
- 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
not overflow two big arrays into an invalid array.
recreate badloader from git history, at 85fa67e8. Could not
reproduce this, but lots of container usage
also got updated in the meantime.
Fixes#683
The testhook/ package provides an easy way to set up multiple
hooks to run before/after tests are run.
The audit hooks track open and closes of storage backends,
files, indexes, and holders, for example. A tempdir wrapper
creates temporary directories which are automatically cleaned up
when the test ends. Any kind of resource creation that
should be closed at test conclusion can be tracked. We
will complain at the end of the TestMain if resources are
leaking.
Leaks under go1.13:
We use a wrapper function which is a no-op for go 1.13, but actually
calls testing.TB.Cleanup in go1.14, so we can still build with 1.13 even though
tests will leak files all over the place there. Because of this,
don't run the testhook tests when using 1.13, as they'll always fail.
- the test/pilosa.go http client now times out after 10 seconds
to help diagnose hung server situations.
- Makefile targets added to get better progress reports.
- Atomic record contains multiple ImportRequest and
ImportValueRequest, plus ability to Clear individual requests.
- adds http handlers for importing AtomicRecord.
- lmdb as a backend (lmdb.go)
(lmdb is the fastest known transactional storage backend)
- per Tx call statics report enabled with PILOSA_CALLSTAT=true (stattx.go)
- framework for per-shard db (dbshard.go)
- txfactory handles any pair under blue-green testing (txfactory.go)
- enable CGO in Dockerfiles for lmdb
Previously, the `rbf.DB.opened` flag was set after `checkpoint()`
when reopening, however, this flag is checked by `checkpoint()` so
it was not properly executing.
This can cause incredibly weird and hard-to-debug problems if the previous
container value is still in the cache after an update, and in particular,
can result in having a stale container value cached after a roaring import
that modified the container. Coupled with another bug which could corrupt
containers on a delete, this produces a very strange bug where a value is
present in a fragment, but an attempt to delete it reports failure.
- 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.
Previously, the `checkpoint()` function determined the segments to drop
based on the current active transactions' WAL ID references. However, if
no transactions are active then the checkpoint would drop segments too
aggressively.
This changes the determination by using the highest WAL ID that is
actually checkpointed to disk to determine the high water mark. If no
page are checkpointed then no segments can be dropped.
green:
TestFragment_RowsIteration/combinations
TestFragment_RoaringImportTopN
red: (needs Ben's attention)
PILOSA_TXSRC=rbf go test -v -run TestFragment_TopN_IDs -tags=' shardwidth20' "-gcflags=all=-d=checkptr=0"
also red: (one for Ben)
TestCursor_FirstNext_Quick/9 is throwing
panic: cannot find segment containing WAL page: 1
as we check the error back from checkpoint() in Rollback().
back to github.com/dgraph-io/badger/v2 v2.0.1-rc1.0.20200709123515-8e896a7af361 b/c github.com/dgraph-io/badger/v2 v2.0.1-rc1.0.20200718033852-37ee16d8ad1c had issues with CI on 386 and arm
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
The new logic to send resize instructions more makes it easier
to hit this, but it's probably always been a theoretically possible
bug to hit: If you are shutting a cluster down, then you stop accepting
connections, which means that if you have an existing resize job, you
can't get responses for it. Which means that the other nodes will
fail to notify you of the success or failure of resize instructions,
so the code waiting on the resize job's status waits forever.
When closing, we bail immediately on that; we don't need to wait for
those notifications. We still have a buffer, and a reasonable confidence
that we'll never write more than one result status, so if one of them
*does* somehow show up and cause the job to have a status,
writing the status won't block.
1. Tests can choose the Tx engine desired by setting the PILOSA_TXSRC
env variable. For example:
PILOSA_TXSRC=badger go test -v -run TestImportClearRestart
2. pilosa server --tx is enabled now.
Examples:
pilosa server --tx roaring # gives the legacy approach.
pilosa server --tx rbf # will activate RBF
pilosa server --tx badger # will activate BadgerDB
pilosa server --tx badger_rbf # will run Blue-Green badger to RBF comparisons.
and so forth. See pilosa server -h or txfactory.go for all valid --tx choices.
3. Mechanism that makes both tests(1) and pilosa server(2) work at once:
pilosa/server/server.go injects PILOSA_TXSRC into env to
communicate with NewIndex in pilosa/index.go.
- 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
Some cluster tests failed sporadically. In order to fix them, I
introduced some debugging-related functionality, which revealed
several new bugs that were actually existing bugs we just happened
not to hit in testing. This combines various fixes.
We start with "make the nodes used in testing have distinct names
based on the test case name", which lets us discover that we are
leaking clusters, which continue to sit around talking with each
other. That in turn causes significantly higher load on access to
ephemeral ports, which causes sporadic failures when we shut a
node down and try to restart it, but something else has gotten assigned
its ephemeral port number since then.
Part of the fix is to try to rebind on port 0 if an attempt to
bind to a specified port over 32k fails. This is a guess; the
actual ephemeral port range could be 16k+, 32k+, or 48k+, or just
about anything else really, but it seems reasonable in
practice.
There were bugs in the oft-repeated loops to await the cluster
achieving a given state, and it could hang forever if it didn't,
so we add a timeout and a standard function on the test.Cluster
type to handle that. Note that the timeout seems irrelevant; in
every case I've tried, a timeout of 0 is fine because the node
start doesn't complete until the cluster state has changed.
Add a method to test.Command to run a query, expecting a specific
result. Also clean up some of the formatting and generation of
queries, and allow parameterized (badly) queries. This lets us fix
a subtle bug, which is that test cases were depending on assumptions
about shardwidths. Also improve the diagnostic output from some of
these functions so test failures are more comprehensible.
But actually that dependency on shardwidths was ALSO revealing a
genuine underlying bug, which is that a node resize did not correctly
propagate the schema to a new node if there was no data present
on shards that node would own. We now also have a test case that
hits that (or would, if we hadn't fixed it).
Add comments explaining the server options parameters for MustNewCluster
and MustRunCluster.
Also, we implement the ReadFrom and WriteTo behaviors for
InMemTranslateStore, without which some of the cluster resize tests
fail. Props to the comment for specifically stating that they wouldn't
work if that happened, which probably saved me several hours of
debugging. The implementations may not be robust, but
InMemTranslateStore is intended to be used only in lightweight
and transient testing.
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.
We don't need the Calls anymore, and especially Precomputed calls
(like Distinct) could be a significant memory load that's increased
as we process additional calls, so we drop the Precomputed references.
We can't drop the calls entirely -- translation can require lookups of
call arguments.
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!
At some point the code changeover to use roaring iterators for
unmarshal got dropped, but the old unmarshal code is way harder to
make work for inspect, so this change is back.
This exports some of the names from the things returned by Info,
but also adds a roaring function to use the unmarshalling logic on
arbitrary data, allowing us to get more insight into a file -- in
particular, letting us distinguish between the bitmaps specified by
the roaring data and the bitmaps resulting from applying the ops log.
- Fix incorrect usage of workspaces (vendor dir in current directory no
longer primary cache of go modules)
- Refactor checkout, github-auth, and mod cache into a reusable command
- Fix issue with dockerhub upload and github authentication
We have a "deadcode" bitmapsEqual which is actually used in testing but
probably shouldn't be, and we don't have a good container equality test.
Problem is, equality tests are sort of slow in the things-are-equal case,
which is the most common case, so we've got some moderately-specialized
code here; specifically, special comparison code that takes advantage
of knowing that if two containers have the same number of bits, you only
have to check whether all the bits from one are present in the other,
because that can't be true for differing containers with the same number
of bits. This reduces the runtime for the ContainerCombinations case
from about 24 seconds to a bit under 2 on my laptop, or from around
10 minutes to about 37 seconds with the race detector on.
Also simplify the InPlaceWrapper functions not to invoke bitmaps, because
it's not really necessary.
BtreeSeek is O(N^2) on its N, and there's not a ton of extra utility
to testing a larger range of values, so we reduce N by a bit, cutting
runtime from ~10s to <1s on my laptop. Also reduce the scale of the
BtreeDelete1/BtreeDelete2 tests a bit because, again, lots of runtime
for little marginal information.
This is pretty expensive even for default shard width, and very expensive
for ShardWidth = 1<<22, and we don't really get much extra benefit from
having a million values instead of a hundred or so.
The generation of slices from things, and use of reflect.DeepEqual to compare
the slices, is a lot more expensive than it needs to be. Omitting it removes most
of the runtime of the marshal tests.
The failure mode in question was pretty predictable and tied to number of
snapshots, not to number of bits written, so we can probably use a lot fewer
bits and still get good results, but this is really slow under -race testing.
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.
The random-value tests can be pathological, and in particular, the
test of arbitrarily-spaced values is in effect O(N^2), and with race
testing on, that test *alone* can take ten minutes to run, but
it's not really all that exciting. We just reduce a bunch of values
and/or test fewer things for these, which doesn't significantly alter
coverage, but reduces test runtime on my laptop with `-race` from
21 minutes to a bit under 5.
When a mapper hits an error, we want it to immediately tell the
other things in that same mapper that they can stop now. But we
don't want to propagate that all the way back up; if a specific
node has a failure executing a query, we will in some cases want
to send a new query to other backup nodes, so the overall
context isn't cancelled yet.
In general, mapFn and reduceFn have been closures that inherit
a context from the function defining them -- but we don't want
that! We want them to be stopped if their specific mapper gets
cancelled, too, because otherwise they can consume a lot of
resources long after the mapper has stopped being interested
in them. So now those are parameters passed into them,
and mapperLocal puts *those* contexts in the jobs shoved into
the job queue, and the workers pass the context in to the
mapFn/reduceFn.
We also check responses from reduceFn now; both mapReduce
and mapperLocal check for a possible error, and return that,
and reduce functions doing anything nontrivial check their
context.
We also add a few more explicit checks for context cancellation
in various places, especially in the GroupByIterator which is
what bit us that one time. The explicit check against ctx.Err
is officially safe as of Go 1.9 or so. (It was previously
unspecified, but on further study, the Go team concluded that
no actual implementation did anything else, and existing code
was already depending on that.) This also affects the rows
function, because that could potentially take quite a while to
run for a large fragment.
I think when this code was written, I thought "freeze" would be
really cheap. It's not actually that cheap. As a result, freezing
things preemptively when it may be that nothing ever tries to write
to them anyway is possibly disadvantageous, to the tune of being
roughly 20% of a sample profile we were shown. Instead, we don't
mark the components "writable", so if anything wants to write to
them, it'll end up freezing itself new copies of their bitmaps
later. But in practice that probably doesn't happen.
This modifies the parser to properly "unquote" incoming strings. So if
a string comes in double or single quoted, we approximately follow Go
rules for removing the quotes and processing escape sequences.
The differences from Go are:
1. we only support backslash, quote, tab and newline escape
sequenences.
2. Single quoted strings are supported and work just like double
quoted strings.
3. The peg parser won't actually accept backquoted strings (I don't
think)
Fixes: #411
We only have 4 bytes for offsets, but what if a file is
over 4GB? Someone came to us with a file with 265 *million* containers,
in a single fragment, which means that over 3GB of their 4.7GB file
is actually just the container headers alone. But we can't easily make
the offsets larger, or change the file format.
So we don't. We just track how many 4GB hunks of the file we've
been through and bump that every time the 32-bit offset wraps. And this
appears to... just work.
This is fixed for both the roaring iterator and the old unmarshalBinary
logic. The logic to handle this will work on 32-bit hosts in the sense
that it will correctly error out for excessively large file sizes or
container counts, but it doesn't actually handle the large files since
it can't.
In addition to adding some tests, this commit moves the
`GenerateUint64Slice()` helper function into a new `generator` package
so that it can be used in both internal and non-internal tests.
We want to be able to control whether or not we use roaring to
serialize Rows, which means serializers have to be able to be
distinct.
We also make corresponding changes to http/handler.go to have
it use the exported serializers directly rather than the API's
serializer (which is always the base protobuf serializer
right now, and if it weren't, that would be bad because we
were assuming it was).
When we're accepting protobuf from a pilosa server, flag that
we'll accept roaring bitmaps as opposed to the naive column
representation.
During testing we spawn a lot of tiny snapshot queues. Make the message
less spammy by printing it only if any enqueues were skipped (shouldn't
ever happen) or more than one thing got enqueued (likely in real usage,
but doesn't happen in testing usually).
In the case where a block merge needed to occur
on a replica containing a row on the edge of the block,
the existing logic would inadvertently clear the first
row in the next block. This PR fixes that.
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.
instead of defining them as being in UTC, but not including the zone
info, we will keep the standard format with zone info, but always
output the time in UTC. This means that we can parse incoming
deadlines that happen to have zone information, though I don't think
we ever need to.
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.
This all needs to be wired into API/Server/Cluster/Holder etc. but I
think the TransactionManager will be a pretty good building block for
managing transaction state at the coordinator level.
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.
If you have two criteria, and the last result you generate is
empty, the nextAtIdx iterator for i==1 will try to continue
poking the i==0 iterator. That one produces a nil result, and
declares the entire group-by iterator done... But the nextAtIdx
call above it isn't checking that, and just loops forever.
This causes some queries to become stuck permanently, consuming
ridiculous amounts of resources almost entirely focused on
calling Intersect millions of times to get empty results.
If you try to Store to a nonexistent field, we create an automatic
Set field with no cache for it, assuming it won't be used for TopN
queries. If you want TopN to work, you need to actually create it
yourself.
If the high end of a range is below the low end of the range, there's
no values in it, so we can short-circuit that. If we don't, if the
low end is zero or higher, and the high end is below zero, we can
get very surprising behaviors, such as accepting values up to the
inverse of the high end. Add a test case for this and treat it the
same as a low range end above the field's maximum or a high end
below the field's minimum, returning an empty row immediately.
If a shard has never had any decimal values in it at all for a
field, the ValCount object returned has no DecimalVal, which could
cause a segfault if we don't check for it. Add a test case which
sporadically triggers that behavior (it's timing/luck related,
unfortunately), and then also fix it.
The computation of available shards is cheap, because realistically, virtually
no one has enough shards that the resulting bitmap is more than one container.
We don't try to fix this at the field/index levels because it's significantly
harder to do there, but I think the creation of these bitmaps is probably
the most expensive part, and switching the unions to union-in-place probably
reduces cost significantly.
Note that the bitmaps being unioned almost certainly have exactly one small
container in them.
We avoid using bitmapContains so often because that turns out to be expensive.
Also, if we produce more than runMaxSize runs, we're going to convert to
a bitmap container (or possibly an array container if there were over
2048 items, but they're all singletons), and we can streamline that by just
converting the source to bitmap and returning differenceBitmapBitmap, which
is faster in this case.
This appears to overall take about half as long in the workload I was
looking at.
The checkptr feature is actually probably right about a few
things in roaring and boltdb, but we can ignore them for now, and
that prevents checking for races, so we disable that temporarily.
Also supply NOCHECKPTR in non-race tests because CI uses "make test"
with -race in $TESTFLAGS and we might do that on other occasions.
Address issues with Distinct failures in testing, or across shards, or in cases where the range of Distinct results is not the same as the range of shards available in any index.
If an index is provided to a bare distinct which happens
to be the index handling the query, then the query needs
to behave as if no index argument was provided.
For example:
When querying against index `i`,
```
Distinct(index="i", field="ints")`
```
should behave exactly like
```
Distinct(field="ints")
```
Problem: A top-level bare "Distinct" call returns results only
for shards on the current node.
Analysis: We don't actually want to limit Distinct calls to "available"
shards at all. We just want to run them on everything. But we already
did that in generating the precomputed results; all we need to do is,
if we get a non-shard-specific request for precomputed values, just
return all the values.
It's pretty hard to create logic for this using our fancy mapReduce,
but also we could just... not do that.
mergeBlock was bypassing the transaction setup stuff, which means that
if we ran out of open files, mergeBlock wouldn't generate ops log
entries (!), also it didn't update the cache (!). This came up because
it also didn't enjoy the "catch your segfaults and issue a diagnostic"
behavior offered by the generation code.
Switch to computing positions directly and calling importPositions,
which does a transaction.
UpdateEvery can change every key, and I think it strongly suggests no
reasonable expectation of repeated access to a previously-accessed key,
but also it can change the containers and replace them.
We were avoiding caching mapped containers in some but not all cases,
and that was causing segfaults. But really, the *problem* is that
the remap operation wasn't clearing (or updating) the cache. Cleaning
that up allows us to take advantage of the caching performance advantage
even when working with read-only/mapped bitmaps.
The only way to hit this:
* Have mmapped containers to begin with.
* Do reads so those containers get frozen.
* Access, either reading or writing, a specific container with key K.
* Snapshot, so the bitmap gets its containers replaced.
* Remember, they have to be frozen -- if they aren't frozen,
we'll update the containers in place.
* Now have GC run so it actually unmaps the data.
* Now try to write to the container with key K *before reading or
writing any other key*. You have to get through the whole snapshot
and GC process without any other reads or writes.
* You get the cached value. You try to use it. You explode.
The sliceContainers code was also setting lastKey to 0 in some cases,
but also setting lastContainer to nil, so this wouldn't have caused
problems, but just to be careful, I've standardized on ^uint64(0)
for everything.
This test is really a test of a very specific bit of the internals
of containers_btree/containers_slice, but we can't easily test it from
there because they don't have all the logic for remapping files.
The underlying issue is that they maintain a single-item "most recent
container" cache, and this wasn't getting updated during the remap
operations, happening through containers.UpdateEvery. The fix is
probably just to make sure that UpdateEvery invalidates the cache.
differenceInPlace wasn't checking for nil containers, which are
theoretically valid empty containers. Also added a couple of other
N==0 checks to streamline the higher-level operation.
- Remove YAML magic
- Remove a lot of duplication
- Update linter
- Use parameterized jobs and matrix build
- Update Docker Hub CD to produce versioned and "latest" images
- Add custom shard width test to workflow
This commit adds `TranslationSources` to the cluster
`ResizeInstruction`. These are the sources of translation
partitions which the receiving node needs in order to support
partition distribution in the new, resized cluster.
This also fixes a bug where index options were not being
encode in the proto Index object. That meant that the schema
transferred via protobuf was not correct. The reason why
things normally worked is because index creation typically
happens on the CreateIndex message, which does include the
options.
TODO:
- [ ] implement the TranslateStore interface for `InMemTranslateStore`
and `mock.TranslateStore`
- [ ] surely need some more tests around the `ReadFrom` and `WriteTo`
If the min/max provided are already on the boundary of int64,
then we don't want to operate on them and cause overflow.
there are still overflow scenarios where a user provides a
min/max which is not on the boundary, but overflow once the
scale is applied. This does not address those cases, but at
least it addresses the default case (where a min/max is not
provided)
When the interval is a proper superset of the range with start equal to
interval start, the range must be considered a superset or it will be
completly ignored (since it neither a subset nor it overlaps)
Co-authored-by: Pierre Fersing <pierre.fersing@bleemeo.com>
It turns out that it's not very useful to keep the sign
value as a separate argument in the pql.Decimal struct.
This commit incorporates it into Value, and makes Value
an `int64` (for some bone-headed reason I had made it a
`uint32` before which is just dumb).
This commit introduces a new type: pql.Decimal
We use that instead of float64 in order to ensure
that the string representation is consistent.
One unfortunate discovery during implementation is
that the RowAttrs and ColAttrs support floats, and
the PEG file was treating them as such. So I had
to split the PEG definitions into float-specific
items and decimal-specific items.
This PR adds support for anti-entropy syncing for integer
and decimal fields. It differs from the logic for other
field types in that it does not rely on a consensus to determine
what the value should be; instead, it considers the correct
values to be those of the primary replica. From there, data
is pushed to all non-primary replicas.
Two changes:
1. Don't write batch/roaring adds or removes when N is 0, because
a write of no bits is not a meaningful write.
2. When unmarshalling roaring things, if a roaring bitmap didn't
change many bits, treat it as having changed at least 1 bit per 8 bytes,
so an 8KB hunk of roaring data counts as 1K changes, which will
nudge us towards snapshotting. This should keep us from having
Large Files show up so much.
This was particularly noticeable on the existence field, which
tends to a steady state of "completely full" very quickly in a lot
of cases.
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
```
In the old unmarshal code, the decision to mark a thing as mapped (always
yes) happens separately from setting the mapping. What if this could ever
somehow possibly go wrong? Let's sanity-check that to be extra careful.
Log an error in the probably-irrelevant case where we ended up with
a file, but Stat failed, which shouldn't ever happen we hope anyway.
Also explicitly discard the status from RemapRoaringStorage in a case
where we don't care.
This is sort of prototype-ish, but the idea is that we use SetMaxMapCount
from syswrap, which already exists, to let us test edge cases like
"what happens if you only sometimes have mapped data".
We might have a problem with a stale mmap, and to try to narrow it down
a bit, we add some sanity-checking features and panic recovery to the
generation Transaction code.
This is pretty experimental.
In some cases, after a snapshot, if mmap fails, we could write
a duplicate of the bitmap to the file, creating cryptic "unknown
op type: 60" messages. This doesn't fix those files, but it stops
making them.
this avoids compounding floating point errors while summing up the
numbers, and means less logic needs to change. Should probably convert
min and max to use this approach as well, though they don't suffer
from the compounding error issue, it is simpler.
this involved adding an optional float value to the ValCount struct
which complicated result types, necessitated grpc changes, and needed
quite a few tests at different layers.
For Fields with ForeignIndex (which have keys), the API was missing
the logic to do that translation against the translateStore of
the foreign index. This commit adds that logic, as well as some
missing translateStore-related logic in the gRPC code.
In the case where a field with a foreign index opens before the
foreign index has opened (and is available as a reference in the
holder), push the field into a queue to have its foreign index
applied once all indexes have opened.
In the `Inspect` function in `server/grpc.go`, getting
the value of an `int` field with a foreign index to
an index with `Keys()`, we need to return the string
key value instead of the BSI int value for the field.
This commit also changes the method `Field.keys()` to be
exported as `Field.Keys()` so that it's accessible in
the server package.
This commit changes the order of FieldOption application so that
it's always set before field.Open() is called.
This was required because field.Open() now uses some of the values
from FieldOptions to determine if/when to use a particular
translateStore. For example, when FieldOptions.ForeignIndex is set,
the translateStore from the foreign index is retrieved during
field.Open().
I ran:
brew upgrade protobuf
GO111MODULE=off go get -u github.com/gogo/protobuf/protoc-gen-gofast
I'm not sure if everything is still going to work, but I'm excited to
find out!
This allows a BSI field to have an option indicating
that it is a foreign key to another index. If the foreign
index has column keys, then this field handles string values
by using the foreign index's translate store.
The lowest limitation I've seen on any filesystem we care about is 255
characters. 230 leaves enough space that an index or field could be
backed up and have a timestamp and file extension appended while
still allowing for much longer index and field names.
This PR adds a `StatusError` to the `pproto.RowResponse` type, which
allows a stream to pass an error on the stream (encoded into
the `RowResponse.StatusError`). This can be checked downstream
for matching `EOF` or `err != nil` and handled appropriately.
This is helpful mainly with the `RowResponse` reducers which run in
goroutines. Instead of trying to manage a separate channel of errors
from those goroutines, we just follow the grpc model and send the
error with the stream.
The check for field existence is not necessary; since we
add the `_id` field to every response then at the very
least that field will be returned.
This check was preventin a query like `select _id from ...`
from returning any results.
This pins us to the initial external release of molecula/ext, which
with any luck will be the only one. (Narrator: It was not to be the
only one.) We also use GOPRIVATE so we don't need a replace directive.
After a few experiments with pkg/plugin, I'm ready to concede that the
people warning me it was unsuitable for production use were in fact
correct.
In the brave new world, the "ext" package is moved to its own module
outside pilosa. This means that importing it doesn't imply any need to
version-check against pilosa; we can just use versioned copies of the
ext package, which can be public because it doesn't contain anything
we need to care about keeping proprietary.
Then we can, conditional on build tags, import modules from a
neighboring repo which contains the actual implementations, and if
they're imported, their init functions register them.
This PR is meant to get all columns from an index
based on the TrackExistence row.
`All()` is a PQL function that can be used as a typical
row object. Optional arguments are `limit` and `offset`.
This PR adds a field name (string) to the return types
which represent the values from a specific field. For example,
a TopN query on field `x` would be `TopN(x)` and have results
like:
```
[]Pair{
{ID: 14, Count: 10},
{ID: 3, Count: 8},
{ID: 7, Count: 3},
}
```
In order to know what field this result type refers to, we wrap
`[]Pair` in a new struct called `PairsField` which contains an
addition `Field` string where `x` is stored.
This is useful for informing the gRPC server how to construct
more appropriate headers for the result stream (in this case,
the column headers can now be "x" and "count").
Similar logic was applied to `RowIdentifiers` and `Pair` as well.
There is a TODO in the `StringWithSubj` method because the value
types really depend on the subject type (for example, `count` uses
uint64, while `sum` uses int64). I'm waiting to address this
until we decide how to handle sums of floats (Decimal), because
that will affect this logic as well.
This PR adds support for a `having` argument in a `GroupBy` query.
Usage looks like this:
```
GroupBy(Rows(a), having=Condition(count > 10))
GroupBy(Rows(a), aggregate=Sum(field=b), having=Condition(sum > 100))
```
In order to standardize results as streams of RowResponse,
this PR introduces two interfaces `StreamClient` and
`StreamServer`) which mirror the grpc stream interfaces.
Upstream users (sqlmapper, vdsm, etc) can implement
instances of these interfaces to ensure that results can
stream through the entire sytem in an expected way.
This PR also fixes a couple of missing data types.
This change allows one to query Pilosa fields and indexes directly
with integer row and column ids even when key translation is
enabled. This was previously disallowed during query
translation... I'm not sure why, but it can be quite useful for
debugging and testing to be able to use IDs directly. I have a test in
go-pilosa which uses this functionality.
I also simplified a bunch of the test code which was of the form:
```
else {
if blah {
}
}
```
to be:
```
else if blah {
```
which I think is pretty harmless.
I also changed a snapshot log line that has been bugging me to be
Debug level so that it isn't generating lots of useless logs for long
running Pilosa instances.
It turns out that we need to recompute the set of shards whenever
a query is cross-index. Otherwise we get partial results in unexpected
ways sometimes.
There's two actual changes here, but they're closely related.
First, handle named parameters for precalls, not just indexed parameters.
Second, when doing translation for a call, check whether it specifies an
index, and if it does, use that index instead of the current index for
the translation.
the threshold.
Prior to this commit, if a cache value was reduced to a value
that fell below the threshold, the operation would be ignored
and the cached value would remain at the old, higher value.
This commit also fixes logic which reduces a cached value within
the framework of uint64 values by subracting the absolute value
of the negative value (since adding a negitive doesn't work with
unsigned integers).
In this case, the test is reading from the translateStore
replica before the translateStore replication has had time to
deliver its log to the replica. The only way to truly address
this in the translate store would be to route all key misses
that happen on a read-only replica to the primary translate
store (or somehow know when the primary is done sending to
replicas) for actual verification that the key does not exist.
That's more involved than we want to do here; this PR just
addresses the problem in the test.
The request for a non-read lock blocks until all existing read
locks exit, meaning that if an Immediate operation is already
going for a fragment, an Enqueue operation will hang forever
holding the fragment's lock, while the Immediate operation has
probably relinquished the fragment's lock to wait for the
queue worker to process it. But the queue worker can't process
it, because the incoming Enqueue still holds the fragment's
lock. Solution: Don't block the Enqueue operation like that.
It shouldn't coexist with things that actually change the sq
channels, like Stop(), but it is fine for it to coexist with
other queue operations.
had to workaround some cruft in the parser that was trying to only
support a BETWEEN query as LTE, LTE. Now we have operations for all
combinations of LT and LTE.
unrelated - changed the port a test was binding to as it conflicted
with a port I was using locally.
this was introduced recently to fix another bug. the comment above it
is correct, just the logic was off-by-one. The test shows the issue
and was confirmed to reproduce it and then fix it.
This is a collection of changes that have been pending forever. It improves the snapshot queue performance, adds some amount of recovery for corrupt filles, reduces memory usage in the rowcache, and adds an extension interface. Yes, they should probably have happened separately over time, things happened.
With the new addition of the holder background scan, it's possible
for an open holder to write log messages at arbitrary times. The
TestHolder_Open/ErrIndexName test checks the contents of the output
buffer, but those contents could be changing if the background task
happens to run at the right time. Use trivial locking around that
so that this shouldn't happen.
The snapshot queue needs a bit more subtlety. In some cases,
we really do want to do a snapshot right now -- these shouldn't
have to wait for possibly a hundred or more other snapshots
to complete.
In other cases, we don't really care that much whether we do
a snapshot, and just dropping it is probably fine.
To accommodate this, we distinguish between "urgent" and
"normal" snapshots, and between "Immediate" (does an urgent
snapshot, waits for it) and "Enqueue" (might enqueue a snapshot
but *also might not* if we're already busy). There's a
corresponding "Await" to wait for a snapshot, if one is
pending, but not if one isn't.
We also have a background scan that checks the holder. It will
scan pretty actively when it's finding fragments that need
snapshots (no enqueued snapshot, opN > MaxOpN). It pauses
for a second after every hundred fragments that didn't need
snapshots, and for a minute after each holder scan that didn't
find any. So, if you don't need snapshots, it does basically
nothing, if you do, it'll be moderately aggressive about
submitting tasks -- but it always waits if there's *any*
requested snapshots in the queues.
Updates since initial draft:
Check results from Await more consistently, and in one case, use Immediate
instead and then check its error.
Fix a race condition. The race condition comes about if:
1. You have a limited enough worker pool that this can happen.
(In testing we tend to have a worker pool of 1.)
2. A fragment is in the normal, non-urgent, queue already.
3. An immediate request comes in for that fragment. This always
happens *with the fragment lock held*.
4. A worker thread grabs that fragment from the queue.
5. The worker thread now waits on the lock. Meanwhile, the
immediate request blocks on sending the fragment to the urgent
queue.
6. The worker can't read the urgent queue, and the immediate
request can't send it, so the immediate request can't proceed.
What's supposed to happen is that the immediate request sends
the thing, and gets into Await(), which sleeps on a condition
variable using the lock, which is to say, releases the lock.
The obvious resolution is to let go of the lock, send the
message, and then reclaim the lock. But then we have the
possibility that the message sent ends up with a timestamp
right after a snapshot that happened *after* the Immediate
request was started. Oops. So we create the request, then let
go of the lock, then send the request, then reclaim the lock
and go into the Await state. All is well.
This is on top of more general use of wait groups, etcetera,
to allow us to ensure that any holder scans terminate *before*
we close the channels they might otherwise be trying to write to.
So, shutdown process is now:
* grab lock on queue (workers and scanners don't use the lock)
* mark snapshotqueue done
* wait for holder scans to complete/exit
* close and nil out all the channels
* release lock
Anything trying to submit to this needs to hold the lock, unless
it's a holder scan, so either it got the lock before we did and already
submitted the thing, or it will get the lock after this and not find
a channel to write to; it's just the holder scanner that has an
ongoing thing that might have started a write to the channel *without*
a lock held, because it's expected that it might have to wait minutes
or hours before the write will complete because it's a background task.
Also, rework the background holder scan to grab lists of
indexes/fields/views/fragments, then scan the grabbed/copied lists,
rather than iterating over maps, allowing us to grab the lock when
we're about to access a thing and let it go when done.
There might be a simpler/cleaner way to do this but opinions on how
safe it is are very mixed, so in the mean time, I'm making the range
behavior not depend at all on there being no writes to the various tiers
of holder/index/view/fragment during the background scans.
So in some cases, when we do a query, the results of one
part of the query are innately shared-across-nodes; for
instance, a hypothetical Distinct query. More generally,
we allow cross-index queries; calls can have "index=foo"
in them.
This patch lets us handle that without duplicating that
query all over. Before we actually start doing the
separate calls, we run the query once from the coordinating
node, then patch the results in, and send relevant subsets
over to each client, etcetera. Also provides slightly
friendlier (and I hope faster) support for converting
bitmaps to/from sets of rows.
We also add an extension interface, and some fancy stuff
to let us define new calls, which use this. They're sort
of tied together because the first extension I wanted to
implement needed precomputed calls. The extension API
lets us create extensions using `pkg/plugin` (with all its
associated limitations, unfortunately), then query them
at load time for functionality.
This also implies some revamping of the argument
validation for PQL, like verifying that functions exist
and knowing things about their argument types.
So basically this is an overly intrusive patch, and would
be better as separate patches, but they're hard to detangle.
add trivial execution-time profiling
What if you could ?profile=true on a query and get some
numbers back? That'd be really cool.
We already have tracing/spans, but right now, those only generate
any data if you have something set up for them to trace to. Add a
fancy wrapper that lets us generate our own tracing data, and dump
it into the request response, if ?profile=true.
add a sample extension, add missing features to extension interface
Implement a naive probabilistic filter extension as an example of
what an extension looks like. In the process, discover multiple
omissions in the bitmap API. Well, I did *say* it was experimental.
This code represents an attempt at providing reliable tracking
of whether any bitmaps still in use have access to a given block
of mmapped data, allowing us to unmap the data when nothing is using
it anymore.
The basic approach is as follows: Each mmap is associated with
a new object, called a "generation". A generation reflects
a particular instance of a given file being mapped. When a
bitmap is built from an mmapped data source, the bitmap is
given a pointer to the generation as its Source. When bitmap
operations combine containers from other bitmaps, they
produce new bitmaps that are tagged with the combined set of
sources.
When we snapshot a file, or for some other reason wish to remap
it, the corresponding bitmap has all its containers updated to
use the new storage, and the bitmap's source is changed. However,
previously-handed-out containers might still have references to the
old storage. Those containers would be in bitmaps with the old
source.
After a bunch of study of trying to reference-count and track
this, I realized: We don't actually need to do that, because we
already have something suitable for determining whether anything
can reach a given object. It's the garbage collector.
So we set a finalizer on the generation object, which handles
unmapping. There's additional sanity-checks here to confirm things
like "we thought this generation should be expiring", and we
track timestamps. We could also have things check whether a
given bitmap's source was marked as obsolete "a while ago", but
that isn't implemented yet.
There's a debug version of this which tracks finalization, creation,
and ending timestamps, and has a call to provide diagnostics for
this. Identical generation IDs get separated out with random
suffixes in this case -- there's sometimes a second or third
instance of the same name due to a holder closing and reopening,
but this basically only happens in testing.
Note that generations are still used even when there's no mmapping,
but unless debugging is turned on, they shouldn't propagate much --
we don't consider a generation to be the source of a bitmap unless
the bitmap actually mapped things from that generation's mmapped
storage, or debugging is on.
There's a couple of other, possibly more subtle, changes and
bug fixes that got caught by the testing on this:
* If a fragment is partially opened and then opening some later
part fails, we close the earlier parts before returning the
error so we aren't leaving it partially open.
* Several operations on segments which were requesting that a
frozen copy of a bitmap be created are now actually *replacing*
their bitmap with the frozen bitmap, rather than discarding it.
* intersectRunRun, if it decides to create an array or bitmap,
will yield that container instead of discarding it.
And why all of this? Why, so we can actually implement the thing
where when a fragment has a valid roaring bitmap, but the ops log
is corrupt, we can truncate the corrupt part of the ops log and
reopen it. Which I did.
When the generationdebug build tag is in use, every generation
has a finalizer all the time. When it's not, they only get finalizers
when we expect them to be done -- say, when closing a fragment.
This is because finalizers appear to be possibly-expensive.
There's some logical cleanup to openStorage here, dividing part
of its work into applyStorage and importStorage, which have a common
case for handling "there's no data in this file".
Which is to say don't actually implement it, because openStorage
is too messy right now, but this is the rest of the framework,
and now I'm going to digress into fixing openStorage.
The available shards file is just a hint to save us a bit
of time later; we don't need it to run and it can get updated
pretty easily later. If we have problems reading it, we
should just report the error, nuke the file, and continue
without it.
Fix an error that could cause imported values to keep high-order bits from previously imported values, and another that could cause BSI fields to store extra bits they don't need.
If you imported only small values, BSI fields could end up
not bothering to clear higher bits in existing values, which
produced strange behaviors.
We also move the computation of requiredDepth, and the change
to the field, down, combining it with the other checks of the
values for min/max being in range.
Without this, a data set with a ludicrously large value in it
could break a BSI field's depth even though the import would then
reject it.
Usage:
`IncludesColumn(Intersect(Row(a=1), Row(b=2)), column=10)`
The above query will return a `bool` indicating whether the
intersection of rows a-1 and b-2 contains column 10. Because
a single column is specified, this executes on a single shard
(shard=0 in this example).
This commit adds a Decimal field type which is implemented mostly with
the Int field. It adds an optional "Scale" value to the Int field
which means that the values stored in that field are actually meant to
be divided by 10^Scale before being interpreted.
In order to make use of this functionality, we extend the importValue
request to allow a slice of floats rather than just int64. If the
slice of floats is present, each float in the slice is multiplied by
10^Scale and converted to an int64 before being imported. If a slice
of int64 is imported to a Decimal field, it is treated normally, and
scale is ignored. This allows the conversion to be handled at the
client side if desired.
Currently there are Field level methods for querying Float values out
of a decimal field, but no support in PQL or the executor for getting
float values. Going to wait until I can use the generic result type
before doing that, so for now, any values queried will be the scaled
integer values.
needed to add client support for importing float values, and did this
by adding a more general and simplified client method for value
imports.
rewrote api.ImportValue to use the new method which should be more
performant and efficient.
allow floats to be "pilosa import"ed into decimal fields
The cluster timeout/down tests are way more than half the total
time for "go test", and are very unlikely to be of interest in regular
usage, although they matter for CI. Skip them when doing short
tests.
Also clean up the license hash checking a bit. We trim vendor early
in find so we don't have to walk the whole vendor tree only to grep
the files out, and we don't check the license hashes of the exceptions,
and the exceptions are now a plain text file of non-regex strings
we match exactly. Also the license hash code is only written once.
This will help us a lot if development on Pilosa continues through
2018 or later.
What if you could ?profile=true on a query and get some
numbers back? That'd be really cool.
We already have tracing/spans, but right now, those only generate
any data if you have something set up for them to trace to. Add a
fancy wrapper that lets us generate our own tracing data, and dump
it into the request response, if ?profile=true.
We track wall-clock execution time, plus possible arbitrary K/V
pairs. Memory stats are not included, because obtaining them is
surprisingly expensive.
add makeRows() tests
register the gRPC server
use api.Index() instead of api.Schema()
support most field types in Inspect() query
currently, there's no support for `time` fields.
those will be dependent upon the output format
and the ability to materialize the timestamp from
the time views.
this commit also changes the response type of the
`Inspect()` query to be a tabular `RowResponse`.
This is the checklist that the reviewer will follow while reviewing your pull request. You do not need to do anything with this checklist, but be aware of what the reviewer will be looking for.
- [ ] Ensure that any changes to external docs have been included in this pull request.
- [ ] If the changes require that minor/major versions need to be updated, tag the PR appropriately.
- [ ] Ensure the new code is [properly commented](https://github.com/golang/go/wiki/CodeReviewComments#doc-comments) and follows [Idiomatic Go](https://dmitri.shuralyov.com/idiomatic-go).
- [ ] Check that tests have been written and that they cover the new functionality.
- [ ] Run tests and ensure they pass.
- [ ] Build and run the code, performing any applicable integration testing.
- [ ] Make sure PR title conforms to convention in CHANGELOG.md.
- [ ] Make sure PR is tagged with appropriate changelog label.
- 'declaration of "(err|ctx)" shadows declaration at'
- 'Error return value of .(.*\.Help|.*\.MarkFlagRequired|(os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*printf?|os\.(Un)?Setenv). is not checked'
All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).
## [1.4.0] - 2019-09-17
This version contains 99 contributions from 11 contributors. There are 94 files changed; 9,453 insertions; and 6,121 deletions.
**Attention**: Pilosa 1.4.0 changes the way that integer fields are stored. The upgrade from old format to new is handled automatically, however you will not be able to downgrade to 1.3 should you wish to do so. We *always* recommend taking a backup of your Pilosa data directory before upgrading Pilosa, but doubly so with this release.
- Add NoStandardView field option ([#1733](https://github.com/pilosa/pilosa/pull/1733))
- Add some stat tracking to roaring implementation ([#1743](https://github.com/pilosa/pilosa/pull/1743))
- Add cluster fault testing using docker-compose and pumba ([#1717](https://github.com/pilosa/pilosa/pull/1717))
- Allow backslash, carriage return in PQL strings ([#1713](https://github.com/pilosa/pilosa/pull/1713))
- Add base system, curl and jq for debug and checks ([#1707](https://github.com/pilosa/pilosa/pull/1707))
- Add `Rows` and `GroupBy` functionality ([#1647](https://github.com/pilosa/pilosa/pull/1647))
- Add `clear` functional option for imports ([#1699](https://github.com/pilosa/pilosa/pull/1699))
- Implement tracking of available shards to help support sparse datasets ([#1600](https://github.com/pilosa/pilosa/pull/1600), [#1695](https://github.com/pilosa/pilosa/pull/1695), [#1624](https://github.com/pilosa/pilosa/pull/1624), [#1663](https://github.com/pilosa/pilosa/pull/1663))
This version contains 65 contributions from 8 contributors (including 1 volunteer contributor). There are 79 files changed; 7,972 insertions; and 2,800 deletions.
### Added
- Set open file limit during Pilosa startup ([#748](https://github.com/pilosa/pilosa/pull/748))
- Add sorting flag to import command ([#606](https://github.com/pilosa/pilosa/pull/606))
- Add PQL support for field values (WIP) ([#721](https://github.com/pilosa/pilosa/pull/721))
- Set and retrieve field values (WIP) ([#702](https://github.com/pilosa/pilosa/pull/702))
- Add BSI range-encoding schema support (WIP) ([#670](https://github.com/pilosa/pilosa/pull/670))
### Changed
- Move InternalPort config option to top-level ([#747](https://github.com/pilosa/pilosa/pull/747))
- Switch from glide to dep for dependency management ([#744](https://github.com/pilosa/pilosa/pull/744))
- Remove QueryRequest.Quantum since it is no longer used ([#699](https://github.com/pilosa/pilosa/pull/699))
- Refactor test utilities into importable package ([#675](https://github.com/pilosa/pilosa/pull/675))
### Fixed
- Add mutex for attribute cache ([#729](https://github.com/pilosa/pilosa/pull/729))
- Use log-path flag to specify log file ([#678](https://github.com/pilosa/pilosa/pull/678))
## [0.4.0] - 2017-06-08
This version contains 53 contributions from 13 contributors (including 4 volunteer contributors). There are 96 files changed; 6373 insertions; and 770 deletions.
*Note that data files created in Pilosa <0.4.0arenotcompatiblewithPilosa0.4.0asaresultof [#520](https://github.com/pilosa/pilosa/pull/520).*
### Added
- Support metric reporting through StatsD protocol ([#468](https://github.com/pilosa/pilosa/pull/468), [#568](https://github.com/pilosa/pilosa/pull/568), [#580](https://github.com/pilosa/pilosa/pull/580))
- Improve test coverage for ctl package ([#586](https://github.com/pilosa/pilosa/pull/586))
- Add support for bit flip (negate) in roaring ([#592](https://github.com/pilosa/pilosa/pull/592))
- Add xor support to roaring ([#571](https://github.com/pilosa/pilosa/pull/571))
The workflow components of these instructions apply to all Pilosa repositories.
## Reporting a bug
If you have discovered a bug and don't see it in the [github issue tracker][5], [open a new issue][1].
## Submitting a feature request
Feature requests are managed in Github issues, organized with [Zenhub](https://www.zenhub.com/), which is publicly available as a browser extension. New features typically go through a [Proposal Process][4]
which starts by [opening a new issue][1] that describes the new feature proposal.
## Making code contributions
Before you start working on new features, you should [open a new issue][1] to let others know what
you're doing, otherwise you run the risk of duplicating effort. This also
gives others an opportunity to provide input for your feature.
If you want to help but you aren't sure where to start, check out our [github label for low-effort issues][6].
### Development Environment
- Ensure you have a recent version of [Go](https://golang.org/doc/install) installed. Pilosa generally supports the current and previous minor versions; check our [CircleCI config file](../master/.circleci/config.yml) for the most up-to-date information.
- Make sure `$GOPATH` environment variable points to your Go working directory and `$PATH` incudes `$GOPATH/bin`, as described [here](https://golang.org/doc/code.html#GOPATH).
- Fork the [Pilosa repository][2] to your own account.
- It will be easier to follow these instructions if you:
```sh
export GH_USERNAME=<yourgithubusername>
```
- Create a directory (note that we use `github.com/pilosa`, NOT `github.com/USER`) and clone Pilosa:
```sh
mkdir -p ${GOPATH}/src/github.com/pilosa && cd $_
git clone https://github.com/pilosa/pilosa.git
```
- `cd` to your pilosa directory:
```sh
cd ${GOPATH}/src/github.com/pilosa/pilosa
```
- Install Pilosa command line tools:
```sh
make install
```
Running `pilosa` should now run a Pilosa instance.
- The official Pilosa repository is your "origin" remote in git. Add your fork as your github username
cd ./cmd/pilosa-fsck && make install && make release
# Run Pilosa tests inside Docker container
docker-test:
docker run --rm -v $(PWD):/go/src/$(CLONE_URL) -w /go/src/$(CLONE_URL) golang:$(GO_VERSION) go test -tags='$(BUILD_TAGS) $(TEST_TAGS)'$(TESTFLAGS) ./...
# Must use bash in order to -o pipefail; otherwise the tee will hide red tests.
# run top tests, not subdirs. print summary red/green after.
# The \-\-\- FAIL avoids counting the extra two FAIL strings at then bottom of log.topt.
GO111MODULE=off $(GO) get -u golang.org/x/tools/cmd/stringer
install-protoc-gen-gofast:
GO111MODULE=off $(GO) get -u github.com/gogo/protobuf/protoc-gen-gofast
install-protoc-gen-go:
GO111MODULE=off $(GO) get -u github.com/golang/protobuf/protoc-gen-go
install-protoc:
@echo This tool cannot automatically install protoc. Please download and install protoc from https://google.github.io/proto-lens/installing-protoc.html
@echo On mac, brew install protobuf seems to work.
@echo As of the commit that added this line, protoc-gen-gofast was at 226206f39bd7, and the protoc version in use was:
@echo $$ protoc --version
@echo libprotoc 3.19.4
install-peg:
GO111MODULE=off $(GO) get github.com/pointlander/peg
@ -348,58 +324,16 @@ install-peg:
install-golangci-lint:
GO111MODULE=off $(GO) get github.com/golangci/golangci-lint/cmd/golangci-lint
install-gometalinter:
GO111MODULE=off $(GO) get -u github.com/alecthomas/gometalinter
GO111MODULE=off gometalinter --install
GO111MODULE=off $(GO) get github.com/remyoudompheng/go-misc/deadcode
Thank you for your interest in contributing to FeatureBase! We appreciate your support in making this open-source project even better. Here are some guidelines to help you get started with contributing to FeatureBase:
1. Familiarize Yourself with the Project:
- Visit the FeatureBase website at www.featurebase.com to understand the project's goals, capabilities, and features.
- Read the documentation available on the website, including the installation guide, configuration options, and data modeling concepts.
- Explore the codebase by cloning the repository and reviewing the source code.
2. Join the Community:
- Visit the FeatureBase community page at https://www.featurebase.com/community to learn more about the project's community and how to get involved.
- Join the Discord server at https://discord.gg/FBn2vEp7Na to chat with other contributors and users, ask questions, and share your ideas.
3. Set Up Your Development Environment:
- Ensure you have Go installed on your machine. Make sure your shell's search path includes the go/bin directory.
- Clone the FeatureBase repository or download it as a zip file from the repository's page.
- Follow the "Build FeatureBase Server from source" instructions in the README file to compile the server binary and the ingester binaries.
4. Choose a Contribution Area:
- Identify the area you'd like to contribute to, such as bug fixes, new features, performance improvements, documentation updates, or community support.
- Check the issue tracker on the repository or the FeatureBase community for open issues or feature requests that align with your interests and skills. Alternatively, propose your own idea by creating a new issue.
5. Create a New Branch:
- Before making any changes, create a new branch in the repository's Git repository. This branch will contain your contributions.
- Give your branch a descriptive name that reflects the nature of your contribution.
6. Make Your Changes:
- Follow the coding style and conventions used in the existing codebase.
- Write clear and concise commit messages for each logical change.
- If you're introducing new features or modifying existing behavior, make sure to update the documentation to reflect the changes.
7. Test Your Changes:
- Run the existing test suite to ensure that your modifications do not introduce any regressions.
- If applicable, write additional tests to cover the changes you made.
- Document any new testing procedures required for your contribution.
8. Submitting Your Contribution:
- Push your branch to the main repository or create a fork and submit a pull request to the main repository.
- Provide a detailed description of your changes, including the problem you solved and the approach you took.
- Be responsive to any feedback or suggestions provided by the project maintainers or other contributors.
- Once your contribution is approved, it will be reviewed and merged into the main codebase.
Please note that by contributing to FeatureBase, you agree that your contributions will be licensed under the Apache 2.0 license, which governs the project.
Thank you for considering contributing to FeatureBase! Your contributions are valuable and help improve the project for everyone.
## Our private fork of [Pilosa](https://github.com/pilosa/pilosa), a distributed bitmap index
- [Docs](#docs)
- [Getting Started](#getting-started)
- [Data Model](#data-model)
- [Query Language](#query-language)
- [Client Libraries](#client-libraries)
- [Get Support](#get-support)
- [Contributing](#contributing)
## Pilosa is now FeatureBase
## Docs
As of September 7, 2022, the Pilosa project is now FeatureBase. The core of the project remains the same: FeatureBase is the first real-time distributed database built entirely on bitmaps. (More information about updated capabilities and improvements below.)
See our [internal documentation](https://internal-docs.molecula.cloud), which includes all [external documentation](https://docs.molecula.cloud), plus many internal-only pages, listed under the "Internal" heading in the main navigation bar.
FeatureBase delivers low-latency query results, regardless of throughput or query volumes, on fresh data with extreme efficiency. It works because bitmaps are faster, simpler, and far more I/O efficient than traditional column-oriented data formats. With FeatureBase, you can ingest data from batch data sources (e.g. S3, CSV, Snowflake, BigQuery, etc.) and/or streaming data sources (e.g. Kafka/Confluent, Kinesis, Pulsar).
For more information about FeatureBase, please visit [www.featurebase.com][HomePage].
The [documentation](https://www.pilosa.com/docs/) for the open source fork is also available, but is outdated for most technical details. Conceptual explanations and [technical blog posts](https://www.pilosa.com/blog/) may still be informative.
## Getting Started
* [Learn how to install FeatureBase Community](https://github.com/FeatureBaseDB/FB-community-help/blob/main/docs/community/com-getstart/com-getstart-home.md)
Optionally, to include Lattice, the in-browser UI, follow the "Build from source" instructions, and run `make generate-statik` before `make install`. When you run a local Pilosa server on the default host, for example, you can access Lattice at [localhost:10101](http://localhost:10101).
0. Install go. Ensure that your shell's search path includes the go/bin directory.
1. Clone the FeatureBase repository (or download as zip).
2. In the featurebase directory, run `make install` to compile the FeatureBase server binary. By default, it will be installed in the go/bin directory.
3. In the idk directory, run `make install` to compile the ingester binaries. By default, they will be installed in the go/bin directory.
4. Run `featurebase server --handler.allowed-origins=http://localhost:3000` to run FeatureBase server with default settings (learn more about configuring FeatureBase at the link below). The `--handler.allowed-origins` parameter allows the standalone web UI to talk to the server; this can be omitted if the web UI is not needed.
5. Run `curl localhost:10101/status` to verify the server is running and accessible.
2. [Start Pilosa](https://www.pilosa.com/docs/getting-started/#starting-pilosa) with the default configuration:
### Data Model
```shell
pilosa server
```
and verify that it's running:
```shell
curl localhost:10101/status
```
Because FeatureBase is built on bitmaps, there is bit of a learning curve to grasp how your data is represented.
* [Learn about Data Modeling](https://github.com/FeatureBaseDB/FB-community-help/blob/main/docs/concepts/concepts-home.md)
3. Follow along with the [Sample Project](https://internal-docs.molecula.cloud/tutorials/getting-started) to get a better understanding of Pilosa's capabilities.
### Ingest Data and Query
## Data Model
* [Learn how to ingest data from multiple data sources](https://github.com/FeatureBaseDB/FB-community-help/blob/main/docs/community/com-ingest/com-ingest-manage.md)
Check out how the Pilosa [Data Model](https://www.pilosa.com/docs/data-model/) works.
## Community
You can email us at community@featurebase.com and [learn more about contributing](https://github.com/FeatureBaseDB/featurebase/blob/master/OPENSOURCE.md).
## Query Language
Chat with us: [https://discord.gg/FBn2vEp7Na][Discord]
You can interact with Pilosa directly in the console using the [Pilosa Query Language](https://internal-docs.molecula.cloud/explanations/pql-intro) (PQL).
## What's Changed Since the Pilosa Days?
A lot has changed since the days of Pilosa. This list highlights some new capabilites included in FeatureBase. We have also made signficant improvements to the performance, scalability, and stability of the FeatureBase product.
## Client Libraries
* Query Languages: FeatureBase supports Pilosa Query Language (PQL), as well as SQL
* Stream and Batch Ingest: Combine real-time data streams with batch historical data and act on it within milliseconds.
* Mutable: Perform inserts, updates, and deletes at scale, in real time and on-the-fly. This is key for meeting data compliance requirements, and for reflecting the constantly-changing nature of high-volume data.
* Multi-Valued Set Fields: Store multiple comma-delimited values within a single field while *increasing* query performance of counts, TopKs, etc.
* Time Quantums: Setting a time quantum on a field creates extra views which allow ranged Row queries down to the time interval specified. For example, if the time quantum is set to YMD, ranged Row queries down to the granularity of a day are supported.
* RBF storage backend: this is a new compressed bitmap format which improves performance in a number of ways: ACID support on a per shard basis, prevents issues with the number of open files, reduces memory allocation and lock contention for reads, provides more consistent garbage collection, and allows backups to run concurrently with writes. However, because of this change, Pilosa backup files cannot be restored into FeatureBase.
Three client libraries were supported for the open source fork: [Go](https://www.pilosa.com/docs/client-libraries/#go), (https://www.pilosa.com/docs/client-libraries/#java), (https://www.pilosa.com/docs/client-libraries/#python). Of these, only the Go client has seen significant use and development in the private fork, and it has now been incorporated into this repository, in [the `client` directory](https://github.com/molecula/pilosa/tree/master/client)
## License
FeatureBase is licensed under the [Apache License, Version 2.0][License]
Pilosa is licensed under the Apache License, Version 2.0.
In addition to these dependancies, you will need to be added to the molecula [Gitlab](https://registry.gitlab.com/molecula) account.
First start the test environment. This is a docker-compose environment that includes featurebase.
make startup
To build and run the integration tests, run:
make test-run-local
Then to shut down the test environment, run:
make shutdown
The previous command is equivalent to running the following:
make startup
sleep 30 # wait for services to come up
make test-run
make shutdown
To run an individual test, you can run the command directly using docker-compose. Note that you must run `docker-compose build batch-test` for docker to run the latest code. Modify the following as needed:
make startup
docker-compose build batch-test
docker-compose run batch-test /usr/local/go/bin/go test -count=1 -mod=vendor -run=TestCmdMainOne .
// NewFileBuffer returns a file buffer which will use an in-memory buffer, until `max` bytes have been written, at which point it will write the contents of memory to a file, and continue writing future data to the file.
// The file will be written to `temp` directory. The buffer fulfills the io.Reader and io.Writer interface
BatchMaxStalenesstime.Duration`mapstructure:"batch-max-staleness" help:"Maximum length of time that the oldest record in a batch can exist before flushing the batch. Note that this can potentially stack with timeouts waiting for the source."`
Timeouttime.Duration`mapstructure:"timeout" help:"Time to wait for more records from Kafka before flushing a batch. 0 to disable."`
