The anti-entropy feature has never actually worked. We've been
talking about removing it or replacing it for ages, but haven't
had a concrete motivation.
But the anti-entropy interface is the sole user of several components
of the Tx interface, and now that we're trying to replace that
interface, being able to drop those components has some appeal, so
let's remove the one thing that used them, in the hopes that this
will simplify life.
This also lets us drop ForEach and ForEachRange, which were
barely used at all. The one surviving usage (CSV export) can be
handled by using the container iterator we already have, and
making ContainerCallback exported so we can use it to just call
things for every bit.
(cherry picked from commit fff9ddc1f5)
* use t.Fatal(f) to abort tests, not panic
* make perf_able run at all, make it debug a bit better
switch perf-able to using same node type we use for other spot instances,
because otherwise it never finds any available capacity.
we switch the perf-able script to use the standard get_value function
instead of direct jq calls.
we try to grab server logs if the restore fails in the hopes of finding
out why the restore very occasionally fails.
* Fix some issues with running IDK tests in docker. (#2248)
*Stop running TestKafkaSourceIntegration with t.Parallel()
This test can't be run in parallel as it's currently written. Doing so
allows for interleaving of messages to the same kafka topic between
tests.
I didn't attempt to modify the test so it could be run in parallel. That
could be done, but left for someone more ambitious.
* Remove idk/testenv/certs which got accidentally committed.
also update .gitignore to include those.
* changes to add bool support in idk (#2240)
* initial changes to add bool support in idk
* modifying some default parameters for testing, will revert them later
* adding support for bool in making fragments function
* boolean values implementation without supporting empty or null values at this point
* Implement bool support in batch using a map (and a slice for nulls) (#2247)
* Implement bool support in batch using a map (and a slice for nulls)
* Keep the PackBools default for now
But set it explicity in the ingest tests which rely on it.
* Modify batch to construct bool update like mutex
The code in API.ImportRoaringShard has a switch statement which causes
bool fields to be handled like mutex fields. This means, that the
viewUpdate.Clear value should only contain data in the first "row" of
the fragment, which it will treat as records to clear for *all* rows.
This makes more sense for mutex fields; for bool fields, there's only
one other row to clear. But since the code is currently handling them
the same, we need to construct viewUpdate.Clear such that it conforms to
that pattern.
This commit also adds a test which covers this logic.
* Remove commented code; revert config for testing
This commit also removes the DELETE_SENTINEL case for non-packed bools,
since that isn't supported anyway.
* Revert default setting
* remove inconsistent type scope
* correcting the logic of string converstion to bool
* resolving an error in a test
* adding tests to cover code related to bool support in batch.go file and interface.go files
* modifying interfaces test
* added one more test case
Co-authored-by: Travis Turner <travis@pilosa.com>
Co-authored-by: Travis Turner <travis@molecula.com>
* resolving bool null field ingestion error (#2254)
* resolving bool null field ingestion error
* testing issues
* adding null support for bools
* updating the null bool field ingestion
* trying to resolve issue when ingesting null value for bool type
* adding a clearing support for bool type
* resolving issues with bool null value ingestion
* updating the jwt go package version and removing changes made in docker compose file
* reverting jwt go version
* removing v4 of jwt
* adding a comment in test file to see if sonar cloud accepts this file
* don't obtain stack traces on rbf.Tx creation
We thought stack traces were mildly expensive. We were very wrong.
Due to a complicated issue in the Go runtime, simultaneous requests
for stack traces end up contending on a lock even when they're not
actually contending on any resources. I've filed a ticket in the
Go issue tracker for this:
https://github.com/golang/go/issues/56400
In the mean time: Under some workloads, we were seeing 85% of all
CPU time go into the stack backtraces, of which 81% went into the
contention on those locks. But even if you take away the contention,
that leaves us with 4/19 of all CPU time in our code going into
building those stack backtraces. That's a lot of overhead for a
feature we virtually never use.
We might consider adding a backtrace functionality here, possibly
using `runtime.Callers` which is much lower overhead, and allows us
to generate a backtrace on demand (no argument values available,
but then, we never read those because they're unformatted hex
values), but I don't think it's actually very informative to know
what the stack traces were of the Tx; they don't necessarily reflect
the current state of any ongoing use of the Tx, so we can't necessarily
correlate them to goroutine stack dumps, and so on.
* fb-1729 Enriched Table Metadata (#2255)
enriched metadata for tables
added support for the concept of a table and field owners in metadata; mechanism to derive owner from http request metadata; metadata for table description
* tightened up is/is not null filter expressions (FB-1741) (#2260)
Covers tightening up handling filter expressions that contain is/is not null ops. These filters may have to be translated into PQL calls to be passed to the executor and even though sql3 language supports nullability for any data type, currently only BSI fields are nullable at the storage engine level (there is a ticket to add support for non-BSI field here FB-1689: IS SQL Argument returns incorrect error) so when these fields are used in filter conditions we need to handle BSI and non-BSI fields differently.
* added a test to cover the keyword replace as being synonymous with insert (#2261)
* update molecula references to featurebase (#2262)
Co-authored-by: Seebs <seebs@molecula.com>
Co-authored-by: Travis Turner <travis@pilosa.com>
Co-authored-by: Pranitha-malae <56414132+Pranitha-malae@users.noreply.github.com>
Co-authored-by: Travis Turner <travis@molecula.com>
Co-authored-by: pokeeffe-molecula <85502298+pokeeffe-molecula@users.noreply.github.com>
Co-authored-by: Stephanie Yang <stephanie@pilosa.com>
We centralize the creation paths for test indexes, fields,
etcetera so they all have a common path, all using standard
test holders. There's still two versions, one for test.* functions
and one for internal. They do share a TestHolderConfig though.
Large hunks of the related APIs are simplified/streamlined.
* Fragments are always created with a Field and don't need
a workaround in case they don't have it.
* Creation of test fragments, etc., use optional FieldOptions
but don't specify names because they're all using new holders
for each thing created anyway. This dramatically reduces
the complexity of the calls.
* test fragments are created inside test views which are created
inside test fields, etcetera, so everything is using the same
logic; test views aren't bypassing the other layers, they're
creating themselves normally within a field.
* Quite a few things now use the standard runtime/production
logic instead of being custom workarounds; for instance, instead
of `mustOpenMutexFragment` creating a fragment and then creating
a mutex vector for it, we just create a mutex-typed field and
have the normal runtime code do this.
* Similarly, we now use the same field creation logic that production
does, instead of having our own test-only thing that validates
field names directly, so our test that we're validating field names
is actually testing the runtime code. Yay.
* fragSpec goes away. it was a replacement for fragProxy which existed
to solve memory allocation problems but replaced them with interface
overhead problems. Now we just have pointers to things and maintain
valid data structures.
* Many panics are now Fatal or Fatalf calls.
* Some specific bugs fixed, like a cluster which was requested and
then had its first node directly overwritten, which isn't valid with
shared clusters.
* Drop the temp-dir test flag and TempDir variable, we can just use
$TMPDIR.
* Drop a benchmark of "write file to disk" that was purely a benchmark
of file write speed, not a benchmark of rendering the data that needs
to be written.
* Drop the unused "flags" parameter to fragment creation, which was
only used back when we changed the BSI format.
* Use holder.Txf() rather than index.Txf(). The TxFactory has to be
holder-level anyway, referring to it via the index is misleading.
* Test holders automatically close themselves and delete themselves,
we remove various other things that thought they were responsible
for deleting themselves.
We reuse a fragment. It might seem surprising that this works, but
the fragment code actually doesn't have a persistent bit depth at
all, it just accepts whatever bit depth you tell it to use. Cutting
out the recreation of the fragments saves some time.
We also cap bit depth at 8, instead of 62, because there's a ton
of runtime cost to testing more bit depths, but it doesn't actually
change the logic.
* removes unused filesize function
* removes ioutil usage
* updates ioutil.ReadAll to io.ReadAll
* updates ioutil.TempFile to os.CreateTemp
* updates ioutil.TempDir to os.MkdirTemp
* updates ioutil.ReadAll to os.ReadAll
* update ioutil.WriteFile to os.WriteFile
* updates ioutil.Discard to io.Discard
* updates ioutil.ReadDir to os.ReadDir where applicable
* removes unused code in idk
* creates type to use for context value keys
* replaces assert.Nil with assert.NoError for error checks
Addressing various lint.
incrementOpN no longer returns errors, because it no longer waits for
the snapshot, so checking those errors is unnecessary.
Several fields in a common embedded structure were "unused" according
to a naive checker.
Other tiny style things, and one actual unchecked error. Yay linters!
As the size of a fragment grows, the cost of snapshots
increases; with a large fragment getting a lot of large writes,
every write will trigger a snapshot, while any other writes have
to wait for that snapshot before they, too, can trigger a snapshot.
To address this, we introduce a background queue of snapshots.
In general, operations which were omitting their ops log writes
and just snapshotting no longer do; they emit an ops log. This does
mean that, in some cases, the ops log is written and then a snapshot
takes place essentially immediately, which costs us some performance.
However, that only actually happens under very light load; under
heavier load, there's generally going to be multiple writes coalesced
into each snapshot, and the ops log writes for them will be much
cheaper than a full snapshot.
When we do a snapshot, we may end up with containers which are
mmapped to the old file, and containers which have allocated storage
identical to the contents of the new file. It would be nicer if they
were mapped to it. But unmarshalling the entire file is expensive.
Instead, we remap it. (Or, if we couldn't mmap it, just make sure
the old stuff is no longer using the old storage space before we
munmap it.)
We add a new ops log type(pair), AddRoaring and RemoveRoaring,
which set and clear the bits from a provided roaring bitmap.
This also compels us to consider additional sanity checking
during tests.
It turns out there's some significant potential improvements to
be had in the case where there's no cache being used on a field, so
we add it to the benchmarks, to allow testing that.
We also make sure that `getUpdataInto` picks the requested number
of columns; if N was a point at which something weird happens,
we might only sometimes see it.
This patch replaces a lot of circumstances in which containers
were being copied with circumstances in which they are shared,
using copy-on-write semantics.
To achieve this, we emulate somewhat the design of go's
native `append` function. Operations on a container may optionally
yield a new container. A container can be marked "frozen",
after which no operation should ever write to it in any way;
that applies both to the container itself and the backing store
it refers to, if any. So for instance, instead of:
c.arrayToBitmap()
we now write:
c = c.arrayToBitmap()
Operations which need to modify a container in any way
need to be able to return a new container, which is a modified
copy of the previous container. This applies to operations
like add/remove, but also to things like unmapping memory-mapped
storage, or changing a container's type.
Bitmaps do not support the same copy-on-write semantics,
currently, but "copying" a bitmap and sharing the containers
instead of duplicating them is *much* cheaper than copying
the containers.
Bitmaps do support a .Freeze method, which currently copies
the previous bitmap, making a new one with the same container
pointers, and freezes the individual containers. Use this
if you need a writeable copy of a bitmap -- the resulting
bitmap can safely have its set of containers modified, and
bitmap operators that would want to modify the containers
will use copy-on-write for that.
The primary motivation of this is to reduce the cost of the
row cache used by fragments. As a secondary issue, the row cache
is no longer updated on writes -- that update was actually a
race condition waiting to happen. Rather, writes to a row
invalidate the cache entry for that row. The row cache is
created by creating a new bitmap, and freezing the relevant
containers from the fragment's storage. In the case where
nothing is being written, the row cache grows to contain
bitmaps containing all those containers, but never copies
any containers. If nothing's being read, the row cache is
never created, and the containers are in general not getting
frozen. The only circumstance where copies have to happen is
when things are read (and thus stored in the row cache) and
later modified. In that case, each read freezes objects, and
the first write to a container after it's been frozen will
create a new copy.
We drop the enterprise/b btree implementation, because we
don't really need it anymore -- we now provide that
implementation by default in the open source product anyway.
Along with this, there's a lot of other changes which
improve support for nil containers, as a cheaper representation
for empty containers. Operations which we know will provide
an empty container can always short-circuit and just yield
a nil *Container. Similarly, operations which would provide
a full container can return a single shared full container
object (which is frozen). The higher-level (non type-specific)
container ops are now using that logic to short-circuit
operations for empty and full containers. (For instance,
difference of anything minus an empty container is the
original thing, union of anything and empty is the original
thing, and so on.)
The Containers interface adds "Update" and "UpdateEvery"
methods, based in part on the "Put" interface provided
by the underlying btree implementation; Update performs
a possible update in-place of a container for a given
key, bypassing the need to replicate the search for that
key in the container. UpdateEvery loops through all the
containers.
Containers do not strictly guarantee that they won't
return nil `*Container` objects. However, the container
iterators won't return those -- empty containers aren't
interesting. Some tests are updated to reflect this.
Some of the container internals, like N(), or the isArray()
and related functions, accept nil container pointers. Some,
like Thaw(), do not. For the array(), bitmap(), and runs()
methods, roaringparanoia enables an explicit panic on a nil
container explaining the problem, but the intent is that those
should never be called unless you already know you have the
right kind of container, so by default they don't perform
the extra checks. In most cases, this is already covered
because a nil container is empty, and there's no operation
we can perform that requires us to inspect the contents of
an empty container. This is passing a fair amount of testing,
but the testing may not be comprehensive enough.
The overall impact of this is pretty trivial performance-wise.
In our default roaring/ benchmarks, a few things get a few
percent faster, or slower. The advantage is that, with
read-heavy workloads, the row cache no longer eats up incredible
amounts of memory.
For a smallish test case, pilosa's memory usage (RES in top) after
startup was ~2.5GB. Without this patch, simply reading every
row a few times got memory usage to about 9GB, which seemed
reasonably stable. With this patch, memory usage went to about
3GB. This will be less noticeable in mixed read/write loads,
but it should be consistently significantly lower.
In addition to dropping things from the rowCache on modifications,
we also stopped performing a full count on a modified row when
not using a cache of a kind that would use that count, and don't
repopulate the rowCache regardless. We don't want every write
to imply a corresponding read after it.
There's a lot of room for possible future optimizations in
terms of things like in-place operations, and some of the
row/rowSegment code is a little suspicious to me, but I don't
think it should be *worse* in any cases.
This commit implements BSI with variable bit depth using a
sign magnitudeto indicate whether a value is positive or negative.
This also rearranges the existence bit to be the first bit instead
of the last bit.
So with the switch to a new linter, we get a lot of new warnings,
and the majority of them are harmless probably, but a few might be
real. Variously just use _ to suppress warnings, or report errors.
There's probably things here that deserve better fixes, but we can
always revisit it.
Data loss was occuring after a cluster restart. The issue was during the
unmarshaling of the op log when multiple values had been written to the log. The
lines in question were like "changed = changed || b.DirectAdd(v)" in which the
DirectAdd would only be executed when changed was initially false, once it was
true, it would never be executed again.
fix large write path—there was a bug because we were iterating backwards over
the small write path to fix that bug, but the large write path needs to iterate
forward. There is enough code difference between the two paths that they are now
two separate methods (which are probably easier to read).
The benchmarks take an absurdly long time to run, and I think these are the
largest offenders. Dropping to two concurrency cases 2 and 16 should give a
pretty good idea.
close files after using them if global max is passed.
I originally implemented this without the global count—just always closing files
when done with them, and reopening for new writes. This was crazy slow for that
one test that uses mustSetBits in a big loop. I modified the test to use
importRoaring and everything worked better (though much more slowly).
After adding the global counter, I ran the tests with that one test using
mustSetBits again, and the performance was similar to master. After completing
this PR, I ran the tests with the max limit set to 5—they still passed but were
much slower.
This provides a simple benchmark that can be used for
setValue, to give a way to compare results from adding BSI
support to roaring. Use the BSIGroup prefix for the
fragments, and specify a cache type of "none", to prevent
the use of a LRU cache (which makes things more expensive).
Add a parallel benchmark for ImportValue, so we can compare
them. (Unsurprisingly, the bulk-import endpoint is quite a
lot faster.)
Also, add a test for clearing values to the TestFragment_Sum
test; it turns out that this was never tested in this code,
but the http client test would test it and verify it, it should
probably also be tested here.
