* Initial commit of code to do a version check-in on startup
* Add json tag to the response struct for version check
* Adjusted version check response types
* Changed error message in version check-in goroutine to use the logger. Changed URL to prod from dev.
* Updated version checkin URL to be analytics
Co-authored-by: Fletcher Haynes <fletcher.haynes@featurebase.com>
* adding kafka consumer config options (--kafka-max-poll-interval, --kafka-session-timeout, --kafka-group-instance-id, --kafka-socket-keepalive-enable, and --consumer-close-timeout)
* wrapping consumer.Close() in timeout. Will wait consumer-close-timeout seconds before forcing consumer to exit
* clean up logs
This commit moves the cli out of the `ctl` package and into its own
`cli` package. It also adds some basic tests for expected input.
Finally, it fixes a bug which was causing intentional line feeds to be
ignored, which was a problem with the BULK INSERT command.
* Database isolation: Balancer
Remove naive Balancer
remove debugging lines
Thread dax.Transaction through Controller
Change role to roleType
Swap out Balancer interface with new one
Standardize InvalidTransaction error
Add some interface comments
* Remove type.Worker; replace with type.Address
* Remove database validate from Queryer
This is already being handled in the `CreateTable()` method. Prior
to doing that validation, we were getting a panic, but that's no longer
the case.
* Remove dax.TableQualifier; replace with dax.QualifiedDatabaseID
* Update IDK test to create database
* performance counters
* first cut of perf counters and system table fanout and a wire protocol
* significantly refactored prometheus support; removed statsd and exprvar
* removed node_id
* put dax subquery test back
* Change Translator.TranslateFieldIDs method to take a dax.TableKeyer
There are a bunch of other calls to the Translator interface methods
with currently take an `index string`, and those need to be converted to
dax.TableKeyer as well. But I need to review each call, because in at
least one place I noticed one being called with `result.Index` instead
of with the qtbl available. And I don't yet know how those could be
different.
Co-authored-by: Travis Turner <travis@molecula.com>
* Change JSON response name from exec_time to execution-time
Execution time stopped working in the CLI because it uses the latest
json tag.
* Wait, don't break the interface.
* Add a test for the sql response json tags.
This is to make sure that if someone like Travis just goes and changes a
tag name to be more consistent, that we perhaps catch that before it
gets to the end user.
* Change exec_time to execution-time after all
* implemented distinct
* implemented distinct
* uses first cut of a buffer pool, and extendible hashing with thresholded spill to disk
* tests
* cleaned up some stuff around query plan output to make developing tooling easier
* added optimization to call PQL Distinct()
* fixed test
* fix for passing wrong index name in orchestrator
* back out change to DistinctTimestamp
* fix other instance of wrong table name being passed
* use full index name instead of abbreviated one for translation. sigh.
* removed some unused code
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
- had to make sure we don't snapshot until directive is fully applied
on a computer... otherwise there's races between loading the files and
truncating the write log.
- added a dirty bit to resources and a bool return to incrementing the
write log... don't snapshot if it returns false because that means
there's been no writes. (but make sure you close the storage transaction!)
- added the actually snapshotting routine which just fires every
<timeout> and serially snapshots everything.
- tweaked some logging
- added ability to get all tables in an org/db or literally all. I
think I just needed the "literally all", but it was natural to allow
it to be scoped to org or DB as well.
* expose Transaction on TranslateStore for DAX Snapshotting
* try to fix ramdisk nonsense
apparently, we were running in either a shell env or docker env
randomly, so this could sometimes pass and sometimes fail since the
shell env had the ramdisk set up and docker didn't.
Now we force to run in docker always and set up ramdisk explicitly.
* ramdisk mount should be defined on gitlab runner config now
* debug ramdisk issue?
* fix tests... and a buncha other stuff
Took retry out of CI config because I think it's doing more harm than
good at this point.
The executor test I modified failed when I changed DefaultPartitionN
to 8, but just because stuff was out of order so I made it more
robust.
I edited some data gen stuff to make shorter lines because it was
making grep results unusable.
the actual fix is in translate_boltdb_test.go
* clean up, fix code review feedback
* delete implementation with test coverage
* optimize IN expressions; stop linter complaining
* fixed some uncovered query cases
* skip test in DAX for now
CLOUD-1252
Implemented Jaffee's fix of checking for b.useShardTransactionalEndpoint
and only running the start/finish transaction block if it's false. Moved
stats timing to a separate defer so it could stay out of the if.
* Fix PQL distinct in dax
When issuing a PQL Distinct() call (or any other call with a "index=" arg),
this commit will attempt to convert the value in the index arg with a
TableKeyer.
* Apply change to call.Children as well
* Add some PQL Distinct (join) test coverage
- check that serverlessStorage is not nil before closing it
- check that we don't already hold a lock on a serverless storage
Manager before trying to load it. This fixed at least one test failure.
These jobs all need TLC and to be moved to the new ansible platform. For
now, we're removing them because if they accidentally get triggered,
they cost a lot of money very quickly, and don't necessarily get us
anything useful if they pass or fail.
Revert "Updating any AWS shape instance to use"
...our existing reserved instance types. This simply ensures if we ever
do run one of these tests it is against existing reserved instances. If
the tests get removed thats OK also."
This reverts commit 69be968d6d.
* Prevent file corruption when writing tar backup to stdout
FB-1794
Tar backups written to stdout were coming out corrupt. This turned
out to be due to log messages getting written to stdout and ending
up in the tar file. We now check to see if the tar file and the log
are both going to stdout, and if they are, send the logs to stderr
instead.
Testing did not have any kind of consistency or validity check. We
now compare a tar file sent to a file and a tar file sent to stdout
to make sure they're the same. This does not guarantee correctness
but does at least catch this form of corruption.
* trying different index name
Co-authored-by: tgruben <tgruben@gmail.com>
Co-authored-by: Todd Gruben <todd@molecula.com>
* Make interfaces more specific than "MDS"
- Introduce `dax.Schemar` interface
- Introduce `dax.Noder` interface
- The rest is generally to standardize on the new interfaces.
- Remove `pilosa.SchemaInfoAPI` interface
- Move `TranslateNode` and `ComputeNode` types from controller to dax package
- Remove `queryer.FeatureBaseImporter`
- Remove `queryer.MDS` interface
- Remove `queryer.Importer` interface
- Identify types using an "MDS" interface and split into Noder/Schemar as necessary
- Changed `Queryer.orchestrator` to a `map[qual]*qualifiedOrchestrator` because we can't share an orchestrator across quals
* Convert orchestrator to use TableKeyer
* Fix "qualifer" misspellings
* Remove `track_existence` and `shard_width` from SHOW TABLES output
* Thread Owner, UpdatedAt, UpdatedBy through SchemaAPI
I took the liberty of renaming "LastUpdatedUser" to "UpdateBy" to align
with "UpdatedAt".
This commit introduces an interface called `TableKeyer` which anything that means to represent a "table"
can implement. Examples are `dax.QualifiedTable`, `dax.Table`, and `string` (for legacy pilosa calls
where Execute simply took `index string`).
In the case of `orchestrator.Execute()` and `qualifiedOrchestrator.Execute()`, we are intentionally strict
about which type of `TableKeyer` the respective method accepts. If we find, in the future, this is too
restrictive, we can loosen that; but for now it helps us understand what is expected.
* Move batch.Importer interface to pilosa.Importer
In addition to moving the interface, it updates all the methods to use
dax.TableID (for example) intead of a string pilosa index name.
* Change unused onPremImporter methods to no-op.
onPremImporter is a wrapper around API which implements the Importer
interface. This is currently only used by sql3 running locally in standard
(i.e not "serverless") mode. Because sql3 always sets
`useShardTransactionalEndpoint = true`, There are several methods which this
implemtation of the Importer interface does not use, and therefore they
intentionally no-op.
* This adds in support to the lattice UI application to use the SQL3
endpoint. If the `/sql` endpoint returns 404, it will use the SQL1
endpoint. If the `/sql` endpoint is available, it will send SQL queries
to that. It does not try the SQL1 endpoint if the SQL3 endpoint returns
an error processing the query. That is, it is all SQL3 or SQL1.
- Below are the specific changes:
- Adds a file that contains functions for interacting with http services as opposed to just grpc/event-based services. As of this commit, it is only the SQL3 endpoint.
- This adds a variable to track if we are using the SQL3 endpoint or not
- This adds a function to handle the response from the SQL3 endpoint
- Adds a function to eventServices to query the sql3 HTTP endpoint
- Fixed a missing semicolon in grpcServices
Co-authored-by: Fletcher Haynes <fletcher.haynes@generalassemb.ly>
* WIP: Convert SchemaAPI to be DAX-centric
* Tables(), CreateField()
* CreateTable(), DeleteTable(), DeleteField()
* More cleanup
* Remove the old SchemaAPI
* FB-1739: Add ability to add a description to a table on creation
- Added CommentOption to handle text after COMMENT option.
- added description field in the createtable plan.
- The description is stored in the existing index metadata.
* Clean up dax service interfaces
Rename some of the `computer` interfaces and organize them in the
appropriate files.
Remove `dax/computer/alpha` package
* Remove ComputeAPI (it was replaced by batch.Importer)
* add nss-tools dependecy to smoke test
* moved the debug code to the right spot
* enforce int min/max constraints on inserts
* add a check for decimal min and max
* fixed borked tests
* fix the decimal to int conversion in constraint check
Co-authored-by: Travis Turner <travis@molecula.com>
* Introduce ServiceManager and Refactor DAX Integration tests
The ServiceManager provides an interface with which to manage
featurebase (dax) services (mds, queryer, computer). It replaces the
confusing interface implementations in /dax/server/server.go (which
optionally used pointers to in-process objects to satisfy an interface)
with (for now) http implementations. The thought is that even if we're
running all services in-process, we should communicate between services
over http in order to mirror what we would do in a production
environment where the services are running on different nodes.
This batch of commits does quit a lot, most of which is captured here:
- Added `path` support to `dax.Address`. Address is now a string of the form [scheme]://[host]:[port]/[path].
- Added `Holder.directiveApplied` to determine (in tests) if the computer has completed applying the latest directive. This is somewhat temporary until we improve the mds-to-computer logic.
- Removed the "service prefix" code which was prepending client URL paths with the prefix. Instead, the serviceType (mds, queryer, computer[n] is now part of `dax.Address`).
- Removed, from the dax config, the top level `StorageMethod` and `StorageDSN` and now just have `MDS.Config.DataDir`.
- Added `Computer.Config.N` to specify the number of computers to run in-process.
- Moved the `pilosa.MDS` interface to `computer.Registrar`. This is an example of getting the interfaces defined in the right packages.
- Added `SnapshotTable()` method to the mds client (to align with its API).
- Changed `Balancer.AddJob()` to `Balancer.AddJobs()` to support, for example, adding 256 partitions in a single call. Refactored some of the naive Balancer to account for this.
- Added a `Seed` to the top-level config. It's not really useful because of package `crypto/rand`.
- Added an in-memory implementation of the DisCo interface and disabled etcd in a computer service.
- Create sepearte data-dirs for each in-process computer.
- Disabled grpc in dax.
- Modified the sql3 test definition format to support multiple insert steps and separate query results (to align with those steps).
* Changes necessary to get multiple computer instance running in-process
For now the config looks like this:
```
[computer]
run = true
n = 4
```
but we can probably just change that to be something like:
```
[computer]
run = 4
```
*Issues found running multiple "computers" in-process*
- grpc was trying to bind on the same port
- changed GRPCListener from `*net.TCPListener` to `net.Listener`
- created a nopListener and set to that for now (i.e. disabled grpc)
- etcd was starting more than once
- changed dax to use in-memory implementations of the disco interfaces (i.e. stop using etcd)
- IDAllocator (which uses boltdb) was trying to open the `idalloc.db` file more than once
- realized we have to set separate data-dirs for each holder. that fixed it.
* Port dax integration tests to ManagedCommand
* Modify Balancer-related methods like AddJob to AddJobs
There were (and still are) a lot of places where we were adding on job
at a time, even when we had a long list of jobs to add. This resulted in
every job add (for example adding 1 of 256 shards) taking ~40ms, or over
10s to create a keyed table. One reason was because each job add was
making multiple boltdb transactions.
* Port over more dax integration test stuff
* Add DirectiveApplied to signify that snapshot/writes have loaded.
We use this in tests to avoid using sleeps.
This should be considered temporary; we're going to need a more robust
solution for determining when a computer node is ready to serve complete
data.
* Finish porting dax integration tests
* Improve godocs
* Remove docker-based DAX integration tests.
* go mod tidy
* Move test/managed.go to avoid package conflicts
* Modify IDK integration tests to work with ServiceManager changes
This is really just computer -> computer0
And the MDS DataDir config change.
* cleanup found during review
* echo $CI_COMMIT_REF_SLUG in CI
* remove docker image arg, use build instead
* fixed a bunch of issues with non-pql aggregation; moved some decimal related functionality; made top actually top (for the non-pql case); experimental create function
* drive up test coverage
implements an fb_exec_requests system table. The purpose of this table is to allow access to internal state to see what queries are running and have been run.
Co-authored-by: Travis Turner <travis@molecula.com>
Reproducible builds are something we should be doing, and we are there
as far as making them in CI is concerned with this change.
The changes to the Dockerfile/Makefile do nothing if the
SOURCE_DATE_EPOCH environment variable is not set before `make build`
happens, or if the build arg is not passed in to docker.
FB-1771
In api_test.go, it was being used to make sure that incorrect input
produced the right errors; this is now handled by making sure the
error isn't nil and then checking its string against the expected
error string.
In executor_test.go and internal_client_test.go, it was being used
to compare QueryResponse structures, which contain an error.
handler.go now has a function specifically for comparing them,
which can provide additional detail if necessary.
Added a test for SameAs to handler_test.go.
* tighten up checks for order by expressions fixed ordering by expressions
* added testing to cover order by cases
* Add DecimalAgg member to proto GroupCount definition
In DAX, where we have split the orchestrator from the executor, and the
orchestrator can run on a different host, there are cases where
`GroupCount`s can travel over the wire via the Internal Client. In these
cases, when the group count contains a decimal aggregate, we need to
send that value as the appropriate type.
* fixed missing cases in order by and case block eval
Co-authored-by: Travis Turner <travis@molecula.com>
stdout/stderr around
A lot of functions in the cmd and ctl packages were passing these
around and barely using them. Replaced them with a logger for most
functions. Some functions get an io.Writer instead so that their
tests can find the output they're looking for.
More cleanup on fb-1766: reworked the tests that were using io.Pipe
or os.Pipe to check their results so they now use a bytes.Buffer.
Unexported some variables that didn't need to be exported.
Fixed NewConfigCommand to use the provided stderr, not os.Stderr.
Added tests for rbf_dump, rbf_page, and keygen, since those weren't
being tested at all.
Added chksum_test, final cleanup.
There are now only four tests remaining which do not pass.
One is related to error format mismatch.
Two require orchestrator work.
One won't pass until table name conversion is supported for multiple
tables.
MarshalLogMessage serializes the log message and prepends additional encoding
information to each message. Currently, we prepend three bytes to each log
message:
byte[0]: encodeVersion - this is currently a constant within the code. If we
modify structs such that they encode differently, we'll have to change the
constant and keep previous versions of structs for deserialization.
byte[1]: encodeType (e.g. "json", etc.)
byte[2]: logMessageType
If we get into a situation where we want more flexibility in these message
header bytes—for example, if we want to use more than three bytes—we could do
something with the first bit of the encodeVersion: if it's 1, that could
indicate that there are additional header bytes, and the following seven bits
could indicate how many.
* Fix formatting in CLI results with custom SQLResonse.UnmarshalJSON
When I started this, it was meant to be a quick fix to address the confusing
result formats we were seeing in the CLI. For example, all large integer values
were displayed in scientifc notation. This is because we were passing the result
types from JSON (in this case, float64) into pretty print. Similarly, `IDSets`
and `StringSets` where being printed using the default go Stringer for the types
[]int64 and []string respectively.
I started by writing a customer UnmarshalJSON() method for the `SQLResponse`
type. Part of this (the part which converts data types based on header types)
was already being used in dax tests, so this just formalizes that logic as part
of the `SQLResponse` type.
Then I realized that the sql3 tests (run against the `sql3` package) were
failing because sql3 is not actually returning the `IDSets` and `StringSets`
types. A future task is to formalize return types, define them, and modify sql3
to return them. Once that is done, we can remove the "typed" switch in the
`SQLResponse` json unmarshaller.
Another significant change is the modification to the `ExprDataType` interface:
```
type ExprDataType interface {
exprDataType()
TypeName() string
TypeDescription() string
TypeInfo() map[string]interface{}
}
```
I added two more methods in order to distinguish between a type (`DECIMAL`), its
description (`DECIMAL(2)`), and its type info (`"scale": int64(2)`). Currently,
the description can be used as the field definition in a CREATE TABLE statement,
but we may want to re-think that. Also, Decimal is the only type currently using
TypeInfo.
Finally, I tried to consilidate things around `dax.FieldType` instead of
comparing against parser types outside of sql3. We still have some sql3 parser
and planner types lurking about, but we can address those in future commits.
* Add some test coverage
* smoke test expected INT, now int
* minor fixes
* Introduce WireQueryResponse and related types
This also changes dax.FieldType to dax.BaseType.
* Populate WireQueryResponse correctly
Currently this is in the http handler, and in the queryer.
* Convert sql3 and dax tests to expect pilosa.WireQueryField in results
* fix PQL tests in the SQL defs
* Address a few of the skipped sql tests in dax
this commit changes the way the plan is retrieved; implements Stringer on types.PlanExpression in preparation for HAVING support; removes last vestiges internal float64 arithmetic; implements a filter on PlanOpFilter; fixes various bugs in the PlanOptimizer when rewriting qualified references
* fixed selects with unqualified identifiers
* handle bad and non-existent query param inputs more appropriately
* added test coverage for PlanExpression Stringer
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
the BackupTar and RestoreTar functionality was ignoring provided
readers, which doesn't matter for real usage but breaks tests
by making them dump raw tar binaries to stdout.
Cobra automatically displays usage messages, and also a gratuitous
"Error: [...]" line in some cases, when any error at all occurs
running a command. To suppress the usage message, you have to set
cmd.SilenceUsage to true. But the code that would do this doesn't
have access to it. To address this, we introduce a category of
"usage error", implemented with stdlib error wrapping (%w) and
use errors.Is to check for it. There's also utility functions
to do this checking automatically, or indeed, to handle wrapping
of the ctl.SomethingCommand and handle running it with a suitable
context and everything.
In fact, several of the places we're checking for usage errors,
we can never actually report one, but we're checking consistently
so that if we want to report usage errors, we can.
For instance, server.Start and (dax)server.Start don't ever
return usage errors, right now, but we're checking their responses
anyway.
The `build lattice` job is a source of frustration because it is a
dependency for building additional jobs in CI, has a wildly varying
runtime--anywhere from 2-7 minutes to finish--and doing a yarn install
&& yarn build is the most CPU/memory hungry part of our CI pipeline.
To address this, I'm hashing the lattice directory, and using the hash
to look for an artifact in S3 which can be downloaded instead of
building the web ui from scratch each commit.
This cache happy path (cached file exists in S3) cuts the time of this
job down to somewhere below 2 minutes even with many concurrent jobs and
pipelines running.
This also gives us the ability to pull built versions of lattice
associated with the commits that produced them, because each new file in
S3 also gets a zero-byte file with the long name of the git revision
(e.g., a zero-byte file called
`924a152ae92372a178bd0fc32b411612b995a6a1` will be stored in S3 so it is
possible to know which commit changed the lattice directory.)
Invalidating this build cache is as simple as deleting a specific key
(or all keys) at s3://molecula-artifact-storage/lattice/<hash> (or
s3://molecula-artifact-storage/lattice/*).
The idea behind this is to give AWS more information about what
instances we can let it actually instantitate, rather than have it be
one fixed instance type.
e.g., in this case, we are okay with any Graviton instance with at least
2 vCPU and 8 GiB memory.
The easist way to do that is to instead use a launch template, with
ec2_fleets or spot fleets.
I took out the part where we even support on-demand instances. This can
be readded later if it is necessary.
* Rename dax.Shard to dax.VersionedShard
* Rename dax.Partition to dax.VersionedPartition
* Rename dax.FieldVersion to dax.VersionedField
* Rename go files to a standard
The IDK tests had extra setup because they needed to work
when not in this tree. They don't still need it. Drop them to
align them with the updated batch tests.
This gets complicated. For coverage output, sonarcloud supports wildcards.
For test output, it doesn't. So we weren't getting meaningful results,
just weird error messages. I fixed that, and got thousands of lines of
other error messages because it wasn't finding the test source files.
That looked like this:
WARN: Failed to find test file for package
github.com/molecula/featurebase/v3 and test
TestTranslation_Primary
But we don't actually need the test reports sent to SonarCloud, because
"which parts of your test suite are being run" is sort of inherently
"basically all of them" with go test. So rather than continuing to do
that, we drop it.
Since we're dropping that, we don't need the JSON output from go test
anymore, so we drop that too, and the tee commands, and the "artifacts"
from the tee commands, and now our test output is human-readable and
slightly faster.
We also bump SonarCloud to 4.7.
We also fix the tests to use GOVERSION sometimes and GOFUTURE other
times, and bump from 1.19.2 to 1.19.3.
Also a couple of minor cleanup (adding explanatory comments,
combining adjacent grep commands, etc.)
Testing unicode is great, but we appear to have had a couple
of cases where we were using strings that weren't valid UTF8.
Weirdly, other instances of these strings work -- I think because
they're in raw quotes (backticks) rather than strings. Anyway,
this is what SonarCloud fusses about.
In this commit, the Directive is mocked; it doesn't actually reach out
to a controller.
Limits key translation to only those partitions (per index) specified in
the Directive. Attempting to create or find a key (or ID) for a
partition which is not handled by this node will result in an error;
translation requests are no longer forwarded to other nodes.
Limits import into only those shards specified, per index, by the
Directive. Attempting to import into a shard which is not handled by
this node will result in an error.
Stub out /directive endpoint
The `applyDirective()` method still needs to be implemented.
Update mds references to use the new /mds/types structure
In mds, we moved the shared types to mds/types. FeatureBase needs to
reference those instead.
This also bumps the mds version in go.mod.
Implement the Add/Remove Index part of Holder.ApplyDirective()
This adds functionality to `Holder.ApplyDirective()` which adds or
removes indexes (tables) based on those provided in the Directive. Still
to be implemented here: shards and partitions.
WIP: remove client from Batch
Move Batch into its own package: batch
Also, in order to avoid import loops, this introduces packages:
/batch/types
/client/types
Reorganize the Importer-related code
Moved the Importer interface to package: batch
Move the "pilosa client" implementation of the Importer interface to
package: client
Modify batch.NewBatch to take an Importer (not client)
This commit modifies the batch.NewBatch() function to use a functinal
option on Batch to inject an Importer into the Batch. Prior to this,
NewBatch() took a pointer to a client, which was a little too
restrictive. Now, MDS can implement an Importer which uses information
from MDS to determine to which node(s) the import calls should be directed.
Add client.SetAuthToken() method to satisfy SchemaManager interface
Update ApplyDirective logic to include fields.
This needs more work, but it was enough to get a basic test passing.
Move Transaction type into /types package.
Add interface check on batch.Importer no-op implementation
Updated ApplyDirective to create all currently support Field types
There are still the following TODOs:
- [ ] impolement field options (ex: decimal scale, int min/max, etc).
- [ ] `time` fields
Added support for Decimal.Scale in ApplyDirective
Update mds dependency
Add /health endpoint
Update to use dax (dax/mds) instead of mds.
After moving the mds repository into the dax repository as a
sub-package, this commit changes everything in FeatureBase to use the
dax repo instead of the now abandoned mds repo.
Introduce and implment the WriteLogger interfaces.
This adds both a `WriteLogReader` and `WriteLogWriter` interface. They
are both implemented by the implementation: `fileWriteLogger`. The
`fileWriteLogger` uses the dax/writelogger API to append log messages to
files on disk.
Add WriteLogWriter.ImportRoaring method to interface
This commit adds the `ImportRoaring` method to the `WriteLogWriter`
interface. Still to implement are the `Import` and `ImportValue`
methods.
Reorganize the ApplyDirective code
The primary goal was to cache the incoming Directive on the Holder prior
to applying all of the changes in the directive (i.e. loading data from
the WriteLogger) because applying those changes often validated against
the accepted state of the node.
Implement all of the WriteLogger read/write methods
Implement the HTTP WriteLogger implementation
WIP: Introduce shard.Version. Implement snapshotter.
Add HTTP Snapshotter implementation
This also recofigures server to use the HTTPSnapshotter instead of the
FileSnapshotter.
Implement snapshotter: TableKeys
Implement snapshotter: FieldKeys
Dependency dance
last of the dependency dance
Add support for prototype
This adds the Makefile targets to build the docker container and push it
to ECR.
SQL3 changes which break with dax changes
Missed TODO: implement FieldVersion version to WriteLogger
Address bug causing missing TranslateStores to error
Originally, we tried to limit the TranslateStores which get allocated to
only those for which the node is responsible. This works when adding a
new table. But if a table already exists, there's no logic to start
missing TranslateStores.
This reverts back to the old FeatureBase logic which brutishly allocates
a TranslateStore for every partition, even if one is not needed.
We need to address this by allowing the ApplyDirective logic to
initialize TranslateStores when they don't yet exist.
Move the ImportRoaringShardRequest type to the types package
Since the ImportRoaringShardRequest object is part of the Importer
interface, we need to move it to a non-root (i.e. pilosa) package. All
the other interface types are either concrete types or part of a
sub-package (such as roaring). We do this to prevent an implementer of
the interface from having to import the entire pilosa package and risk
circular imports.
buncha changes to support latest dax stuff
Move dax related types to /dax sub-package
This commit moves all the common "dax" types into the /dax sub-package.
The idea is to ensure that featurebase does not import dax at all.
It's ok if dax imports featurebase.
In the future, we might need to split the dax sub-package (common data
types used by muliple molecula data-plan services) into it's own repo.
Add type: dax.Schema
This isn't currently being used; I started to use is as a replacement
for pilosa_client.Schema, but then deferred that. But we'll need to do
it eventually, so it doesn't hurt to have this type in place.
Export RowIDs.Merge() method for use in orchestrator.
Add CreateSQL method to dax.Table type
The CreateSQL() method will return the "CREATE TABLE" statement required
to create the dax.Table.
Comment out confusing writelogger log message.
We need to revisit this, but for now, this log message is confusing.
Also, rename daxSharder to versionStore.
Remove hard-coded AWS account
Implement more FieldOptions such as Epoch
Some of the FieldOption logic was stubbed out in the dax package. This
commit fills that out more; specifically, it adds the
dax.Field.Options.Epoch parameter.
export stuff needed for TopK in orchestrator
export ValCount stuff to implement Percentile in orchestrator
export more stuff to support less code in orchestrator, shared objs
Port dax repo over to featurebase/dax (run all as sub-services)
This commit does ALOT. Sorry.
It introduces a `featurebase dax` sub-command which can be configured to
run the various dax services as sub-services within the same process, or
individually as the lone service in process.
It also changes all the URL paths to be prefixed with the service name.
So for example, instead of calling localhost:8080/status, you would now
call localhost:8080/featurebase/status.
Also, note that all services provide a /health endpoint to confirm they
are running in process.
Clean up integration tests. Remove PILOSA_ config prefix.
Remove duplicate clients (mistake from porting dax to featurebase)
Rename sub-service "featurebase" to "computer"
In the places where we have hard-coded the sub-service name into a URI
path, I've tried to tag the line with a comment containing:
`// #SERVICEPATHPREFIX`
Update copilot manifest files to reference "computer"
Port dax/README.md from dax repository
Separate (toml) Queryer Config from Injections
We needed to separate the toml config from the configuration required to
inject sub-services into the Queryer. I'm not sure this is the best
solution, but it's *a* solution. So here we are.
Clean up (i.e. remove) the queryer "implementations" package
Remove old test file
Run WriteLogger and Snapshotter as local sub-services.
Prior to this commit, the writelogger and snapshotter services only
worked when run as separate services. This allows them to be run in the
same process as all the other dax services.
There is still some naming issues that we should address, but it's
functional for now.
Clean up (i.e. organize) the intra-service interfaces.
Implement alpha Director for local messages from MDS to Computer
Prior to this commit, messages from MDS to the computer service were
still going over http. This commit introduces an interface
implementation which registers the local computer command, and use that
command's API to directly reference methods used by the Director.
Clean up a few more interface names
Add Queryer OpenAPI document.
Update copilot manifests to reflect latest changes
Add OpenAPI documents for WriteLogger and Snapshotter
Add OpenAPI document for MDS service
Add OpenAPI document for Computer service
Consolidate errors to use fb/errors package.
This commit is a first pass at trying to ensure that all of the DAX code
uses:
"github.com/molecula/featurebase/v3/errors"
This package is a wrapper for "github.com/pkg/errors", so going forward
we want to avoid importing that package.
The only method which isn't backward-compatible is `New()`; the
New() method in the featurebase/errors package takes an errors.Code. If
this becomes a problem, we could change this by reverting New() and then
introducing something like NewCoded(). But for now I think it might
actually discourage someone from just creating a New() error without
thinking about how it should be coded.
Introduce VersionStore interface
Move the existing VersionStore code to the `inmem` package as the
in-memory implementation of the new dax.VersionStore interface.
Introduce NodeService interface
With this, the Controller can maintain a registry of nodes by using this
NodeService interface as opposed to an in-memory map of nodes on the
Controller struct.
This also adds an inmem implementation of the NodeService interface.
Introduce controller.Balancer interface
This moves the existing balancer package to controller/naive package.
The idea is to allow us to add a different Balancer implementation in
the future.
Introduce DirectiveVersion interface
This commit also includes *A LOT* of refactoring to use dax.Worker and
dax.Job types everywhere instead of strings.
Introduce Schemar interface
The previous `Schemar` struct was moved to the `schemar/inmem` package,
and `Schemar` is now an interface implemented by that inmem package.
Remove unused type `nUnit`
Add boltdb implementation of VersionStore interface.
This removed the previous sqlite implementation; we decided not to use
sqlite for now (as a basic, local disk implementation) because it
requires CGO.
--------------------------------------------
No longer applicable:
Add sqlite implementation of VersionStore interface.
This commit implements the VersionStore interface using sqlite. Sqlite
requires CGO, so this may not be something we want to include, but it's
implemented here to get a feel for how an external implementation might
be used; the next step will be to determine how the user configured
FeatureBase to run using sqlite as a backing store for services like
MDS.
Add boltdb implementation of NodeService and DirectiveVersion interfaces.
Add boltdb implementation of naive Balancer interfaces.
This includes the two interfaces defined in `naive/balancer.go`:
- WorkerJobService
- FreeJobService
Add boltdb implementation of Schemar interface.
clean up a linter issue
Thread context.Context through all the interfaces.
Some of the interface implementations are going to use context, so we
need to make that part of the interface. The boltdb implementations, for
example, take a context. This is probably so we can do things like
cancel or timeout operations.
Update interfaces to return error; remove `panic(err)` everywhere.
Down-rev grpc version to 1.38.0
Later versions (after 1.42.0?) cause MustRunCluster.Close() in tests to
deadlock.
This commit also adds an `isComputeNode` feature flag around some of the
write log and shard/partition check functionality so that it doesn't run
under normal conditions (this is excercised by running the sql3 tests
for example).
Add MDS_Persistence test to cover meta data persistence
This adds a basic test which configures the MDS container to use boltdb
as its persistence storage, saved on a docker volume. Then, the mds
container is stopped/replaced, and we confirm that the data stored on
the volume is availble to the new MDS container.
Fix a few things after rebase with sql-experiment branch
The lastest version of sql-experiment contains a fairly significan
refactor of the way query iteration works. This commit adjusts for those
changes.
pull dax IDK changes in to FB IDK (#2177)
* pull dax IDK changes in to FB IDK
* Move docker-related IDK build stuff to featurebase root
Building the docker image required the root level go.mod and vendor
directory. This change moves the make targets to the root level
Makefile, and the Dockerfiles now copy the root level vendor directory
(and everything else in the root for that matter).
* Fix batch- and client-related tests
* InitializePoller on MDS restart/replacement
Prior to this change, if MDS was restarted, its internal poller (which
maintains an in-memory list of nodes to poll) is empty. This is bad,
because it doesn't know about nodes that it should be polling.
This change fixes that. Upon MDS startup, it intializes the poller with
the list of nodes that MDS keeps in persistent storage (currently:
boltdb).
* Add EFS volume to MDS Copilot manifest
This allows us to use MDS's persistent storage (via boltdb) in the
Copilot demo by saving metadata in a boltdb file on EFS.
* Thread logger.Logger through all dax components
* Revert some of the breaking changes from DAX development.
When we first started prototyping DAX, we made changes to the
featurebase core code which would have broken the existing featurebase
functionality. This commit reverts some of those changes. Anywhere that
we need to modify core featurebase functionilty, we put it behind some
kind of feature flag. This flag is typically determined by whether the
running node is a "compute" node (i.e. DAX.COMPUTER.RUN = true).
Co-authored-by: Travis Turner <travis@molecula.com>
add packaging for DAX
need cgo for datagen build
bind to 0.0.0.0, pass GOOS and GOARCH explicitly
not sure if the explicit GOOS/GOARCH is actually necessary...
Get INSERT INTO (aka ingest) working through SQL3
This commit does a few things which I'll try do describe here.
- Introduces a Qctx interface. The existing Qcx is an implementation of
this interface, and can be used exactly how it has been. But this
allows us to abstract away the notion of Qcx in the Queryer (which is
handling SQL3) until we're ready to address that. As an example, the
Qcx has a notion of a featurebase Holder, but that doesn't make sense
when we're at the Queryer layer. For now, the Qctx used in the Queryer
is a no-op.
- Adds a ComputeAPI interface implementation for the Queryer. This is
effectively the Import() and ImportValues() methods used for ingest.
The logic here handles the incoming ImportRequest by first doing any
necessary column and row translation for the entire request, then it
splits the records by shard, and generates a new ImportRequest per
shard with only the shard-appropriate records.
- Changes the mds.Importer to take an MDS interface implementation
(which can be an mds client) instead of an mdsAddress. This allows us
to use a localy MDS implementation rather than assuming we need a
client to make calls over a network.
Add queryer.Importer interface to handle ingest via SQL (#2203)
* Add queryer.Importer interface to handle ingest via SQL
This is meant to support ingest through SQL when the queryer and the
compute services are running in the same process, or when they are on
seperate processes and need to talk via http client.
* remove datagen from RPM
was originally added as a convenience to generate test data, but is
unused and annoying because datagen doesn't easily cross-compile due
to cgo
* add marshalUnmarshal to controller to avoid passing pointers
passing pointers across API boundaries can cause unpredictable things
in local vs remote configurations.
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
"fix" a few issues with wrong default partition numbers
these still need to be properly fixed and actually get the correct
data from MDS
go mod tidy
Introduce TableQualifier (OrganizationID/DatabaseID) (#2220)
* add check in ApplyDirective that version is increasing
fix TestAPIDirective to make version always increasing
* fix docker image build and break out dax test in CI
We have to run the DAX integration tests separately as they call out
to Docker, and so it isn't easy to run them in a Docker container as
the other tests do. So we run them directly on the CI runner which has
Docker and Go installed.
We also explicitly exclude these tests from running during the other
tests.
Also my editor was automatically reformatting some comments badly
which is why I added the "data" thing in those two places
* add timeout to poller
* give Poller a default Logger
apparently we can NPE sometimes, seen in CI: https://gitlab.com/molecula/featurebase/-/jobs/3028286364
* bunch of testing fixes, mostly IDK/DAX related
make MDS error if sendDirectives errors, don't just
log. sendDirectives can error if computer nodes disagree about the
validity of a schema (for example), in which case it might need to get
deleted and user notified somehow. very messy, needs more thought.
re-introduce old env prefix to maintain compatibility with master
branch
make self-contained dax container for IDK testing
build IDK images from source (now that all the source is available
since it's in the same repo)
catch errors in DoExtractQuery in idktest.go
fix IDK bug where prefix path was hardcoded in all cases rather than
only when useMDS was true
fix TestBatchTargetMDS... needed to add field options and catch error
when creating table. also needed an _id field
* fix env prefix in tests
* WIP getting tests to pass, wanna see CI
* don't error if we get a zero version directive and we don't have a
directive yet
* cleanup debugging junk
* "fix" future.rename thing, run IDK tests
* Introduce TableQualifier (OrganizationID/DatabaseID)
This commit introduces a lot of new types (in dax/table.go) related to
TableQualifer (which is made up of OrganizationID and DatabaseID), as
well as things like TableID and TableKey.
For the most part, we try to thread a QualifiedTableID through the
entirety of DAX. There are some places (for example in the Balancers,
which are just aware of string keys) which use a string TableKey
(tbl__org__db__tableid).
* Remove some debugging comments
* Add Org/DB support to CLI.
This commit adds support for special commands:
SET
SET ORG acme
SET DB db1
USE db1
* remove ".pulled" from IDK Makefile
I don't think we need it any more as most things can be built
locally. I think it was only there to refresh the FeatureBase images
that were tagged as master, but we don't need to do that any more.
* Change DAX json tags to kebab-case (i.e. hyphenated)
This commit also renames some struct arguments to more accurately
reflect their type: for example, renaming `Table` to `TableKey` when the
type is TableKey.
* Return DAX TableName in SHOW TABLES (instead of Index.Name)
There are cases where SchemaAPI is used to return DAX friendly table
names (as opposed to featurebase index names, which are DAX TableKey).
This is an attempt to do that. With that said, it's not ideal because
anything could call those API methods and expect the other type.
* Fix a bug which wasn't completely dropping a table.
When using boltdb as a backend, DROP TABLE wasn't removing the
reverse-lookup key for the table in boltdb.
* Remove idk/testenv/certs which got accidentally committed.
also update .gitignore to include those.
* Fix IDK ingest tests to be TableQualifier aware.
* Add example Table types to dax/table.com godoc.
* ignore idk.Main fields for flags, upgrade commandeer
* go mod tidy
* Fix DAX integration tests: ingester using wrong ENV VARs
We change from ORGANIZATION_ID to ORG_ID
and from DATABASE_ID to DB_ID
* Clarify things around idk (docker) tests
* 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.
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
Require Directive.Version be a non-zero value. (#2227)
Because the directive cached on the holder is not a pointer, its default
version is 0. In order to avoid having to compare against that, we just
require that Directive.Version start at 1.
General, non-invasive code cleanup and comment adjustment.
Move ImportRoaringShardRequest out of the types package
Early on in the DAX development, I moved ImportRoaringShardRequest into
a types package. There must have been some import loop going on, but
since that is not longer the case, it's safe to move this back into the
core featurebase (er... pilosa) package.
Move Transaction struct back into the pilosa package (from types)
Revert some name changes (cli -> client)
Add DAX Handler CloseTimeout
This was implemented in htt_handler.go, but it had been commented out in
the DAX handler. This just uncomments that and finishes the
implementation.
Remove Qcx from queryer.Importer interface
This sets us up to revert the Qctx interface that was initially
introduced to allow us to abstract away the need for a Qcx when calling
the ComputeAPI from a remote service (i.e. the queryer).
Add some go-doc comments and remove unused code.
Move SchemaManager setup from datagen to idk.Main (#2233)
The set for idk.SchemaManager (for dax implementations) was previously
in datagen. This may have been because of some import loop problem
during development, but that's no longer an issue.
The setup for this should be in idk.Main so anything using that can
leverage the MDS-specific SchemaManager setup.
Fix issues around nil TxFactory
First, don't return a nil. Rather return a new *TxFactory (with no
holder).
Second, don't call `f.holder` in the testhook outside of checking if
`f.holder` is nil.
Wrap all bare errors
Make service prefixes constants
Instead of having `"computer"` throughout the code, use instead a
constant: `dax.ServicePrefixComputer`.
MDS skip errors when sending empty directives
also add in the docker-login and ecr-push changes for serverless DAX
Fix the logic in Directive.IsEmpty() (#2236)
Update the cached value for Index.translatePartitions
In the case where a node already knows about an index, but its
assignment of partitions for that index changes (for example, when
another node goes down and the node in question is now responsible for
more partitions than it previously was), then we need to update the
cached value of Index.translatePartitions because that's used in
translation checks.
minor fixes for IDK-related bugs
WIP: tokenize CLI to access cloud
FB CLI cloud support with automatic token refresh
Also adds support for a GET command which allows making HTTP GET
queries to cloud CP which can be handy for debugging stuff. E.g. GET /v2/databases
buncha little fixes working on writelogger stuff
fix writelogger/snapshotter setup bugs
implement writelogging for importRoaringShard
add debug endpoint to MDS
use shard transactional endpoint in MDS datagen
add debugging to API related to writelogger
revert handleroption change
clean up big PR
remove "GET" command from CLI for making arbitrary HTTP request to
cloud control plane (was a messy hack and not that useful)
remove json tags from FB objects where we had to duplicate the object
elsewhere due to import loops and weren't actually json encoding it
unexport handlerOption which was exported to try to avoid doing
certain things if we're in DAX mode, but I didn't end up merging that code.
remove (hopefully) unecessary extra call to api.indexField
fix some formatting, unexport some vars, godoc, etc
oops, fix build failure
Update FeatureBase CLI to support a standard deployment
The standard deployment uses a different endpoint and request payload.
This commit tries to detect is the standard deployment is being used,
and if so, it uses a standard-specific FBQueryer.
It also modifies the auto-detection logic to try standard featurebase
and dax ports in the case where a port was not provided.
MDS API refactor (#2259)
* MDS API refactor
table IDs are exposed but only created server side
also cleaned up dax Makefile
* clean up review feedback
Co-authored-by: Travis Turner <travis@pilosa.com>
* remove TablesByName
* rip out inmem implementations and use boltdb everywhere
* remove inmem balancer, create bolt tempfile by default on startup
* WIP on snapshot table impl and test
* Minor comment and code layout adjustments.
This commit also adds the `Equals` method to `QualifiedTableID` for
equality comparisons. It's no longer safe to compare struct (two structs
might still be equal even if one of the structs doesn't have a `Name`
value.
* Use a unique docker network for each dax test
Ocassionally we would see some test failures due to a network already
existing. This shouldn't happen, but to avoid that, this commit
generates a unique name for each sub test (which gets deleted at the end
of every test).
* Fix one instance of NewQualifiedTableID losing Name
We should probably check the other instances and see if Name is getting
lost.
* simplify unique network stuff and fix api directive tests
* Remove TableIDRequest and TableIDResponse types for /table-id (#2267)
For the mds/table-id http requests, just use dax.QualifiedTableID as
both the request and response types.
* remove lattice from dax, no error on node re-reg, dax docker-compose
* various updates
* WIP: mds-refactor branch review
* no-op on SnapshotTableKeys if table is not keyed
* Makefile helpers
* add doWeCare so controller doesn't fail unnecessarily
* clean up table creation (#2272)
* Strip underscores from TableID stub name
* fix boltdb versionstore tests: generate unique, sorted tables
* fix controller test related to reregistering a node
* JobSet -> generic Set
Co-authored-by: Travis Turner <travis@pilosa.com>
Co-authored-by: Travis Turner <travis@molecula.com>
Cleanup after rebase on master
The latest rebase on master entailed all the client/batch changes as
well as some of the qcx refactoring. It made for a hairy rebase. This
commit fixes some of the tests that were failing after that rebase.
Fix batch/client import loop missed during rebase (#2280)
It's not surprising that `batch` can't import `client`. It was doing
that here (importing an error type from the `client` package). What is
surprising is that it's okay for `batch_test.go` to import `client` even
though `batch_test.go` is an internal test and therefore part of the
`batch` package.
different boltDB's for schemar/controller, explicit balancers
nice helpers for dax docker-compose, make build really fast
build FB binary outside of docker, then create Docker image with its
working dir in an empty subdirectory so it doesn't send a GB of
context to the daemon.
error on unassigned jobs and use client with timeout
fix CR feedback
deregister batch of nodes
also make removal faster via director dial timeout
implement WorkersForJobPrefix so orchestrator doesn't make up shards
also fix some godocs and remove unused method
Run sub-tasks of a Directive concurrently in a worker pool. (#2275)
* Run sub-tasks of a Directive concurrently in a worker pool.
This allows the compute node to concurrently load shapshot and writelog
data concurrently, instead of one keyset/partition/shard at a time.
It introduces a config parameter called `DirectiveWorkerPoolSize`.
* code review cleanup
* Use unique container names in DAX integration tests
We were seeing "container already exists" errors in CI, so just to be
safe, this commit constructs a unique container name for every container
in the DAX integration test run.
Stub in SystemAPI to Queryer (note: will not work if used)
This just makes is so that dax can compile. Actually implementing
system-table functionality for dax will take some planning.
Tlt/dax merge prep (#2282)
* Remove copilot directory
* Remove Dockerfile-datagen-long
* Remove orphaned RegisterNodeRequest
This type is not defined in the dax/mds/http package.
* implement TIMEQUANTUM and TTL in Table.Field type
* Remove the "service" misdirection in queryer/writelogger/snapshotter.
We had originally used an additional layer, er.. package, for a "service".
The main distinction was that the Config differed in that it was
internal, unlike the Config that we need to provide for the top-level
server config (i.e. toml). Having that additional layer just to support
a different Config seemed premature at best. So I'm removing it.
* Remove dax docker containers no longer used in tests
Since we run everything as "featurebase", we don't have multiple
container types anymore.
* Some minor comment updates
* Remove nfpm stuff related to dax
* Fix linter issues
Fix "duplicate" issues raised by sonarcloud.
run docker components of dax integration tests with coverage
trying to get dax integration coverage
add coverate volume mounts throughout dax integration tests
add a lock, tweak dax Makefile, remote flag on query handler
remove some unused code
convert batch tests to use clustertests to get coverage
maybe fix clustertests
more authclustertests fixes, test is failing locally
but also seems to have been silently failing in CI prior to these
changes... let's see if it's still silent
fix some lint to kick CI
just re-running the job wasn't working... strange behavior
remove RetryLogic test and pipe which don't work
RetryLogic test removed due to etcd changes. Seebs thinks we shouldn't
test this here.
Pipe was being ignored since we're no longer using "bash -c" to
execute the command. If we need to generate that output file we'll
either have to reintroduce bash -c and set -o pipefail so that it
actually fails properly, or figure out some other solution.
shooting into the dark...
first cut at bulk node registration
remove unused stuff from batch tests, set coverpkg to ../...
batch registration timeout and fix tests
disable most tests and don't run fb background batch test
debuggin!!!!!!!!!
and then he tried this....
Implement importer (for INSERT INTO) in the Queryer
Prior to this, we we passing a nil value in for the importer to the
planner.NewExecutionPlanner in the Queryer. This meant that INSERT INTO
statements didn't work. Now they should.
It uses the importer that we build for IDK in /idk/mds/importer.go, and
wrapps that with a type that can determine if the provided string
"index" is of the form indexName or TableKey.
turn off debug mode, fix log saving
Run sql3 test definitions in a dax integration test
There are currently 22 tests which are not passing. They are skipped in
the "skips" slice.
WIP, not working, pql queries to tests
Add TableQualifier to PQL query logic in the Queryer
Add more PQL tests to the keyed table
Allow instant node registration if registration-batch-timeout=0
When running dax services in process, we don't want to wait 3s for the
compute node to register; we know it's there because it's in the same
process.
Fixes related to IncludesColumn PQL test.
Tests for ConstRow and FieldValue
cleanup
add UnionRows and Options, better error reporting on bad queries
delete unused schemar client.go, clean up unused in batch test CI
move test timeouts into more reasonable territory
apparently this had already been done, but got merge-stommped at some point
move dax bolt test helpers into dax package
Add computer CheckIn routine (#2296)
* Add computer CheckIn routine
This adds a background routine which sends a "check-in" request to MDS
every <interval>. This is to address the case where the poller has
removed a computer node from the node list (due to a network fault, for
example), but the node is still healthy and becomes available again. In
that case, the node needs to "check-in" to tell MDS it is still there.
MDS will likely send the node a new directive with Method=reset telling
the node to delete all of its data an apply the latest directive.
* Don't send directives to Deregistered (i.e. removed) nodes
We have an issue where we're locking on sendDirective in the
controller, and when the node is unavailable, the send hangs and never
releases the lock. This is a temporary fix for that until we address the
real problem.
Fix .gitlab-ci.yml after rebase
fix some indentation shenanigans
Concurrency was previously fully disabled due to duplication when using '--auto-generate' but testing has shown that it works correctly when not using that flag. Added the required check and updated help text
At 50ms, we see sporadic failures in CI. So much for "this should
only need a couple milliseconds". Bumped timeout to avoid that.
The challenge here is that we have some tests which *want* to hit
the timeout to confirm that we aren't allowing things we shouldn't.
But we don't want the test to hang forever. But we want to be sure
it is actually stuck and not just being slow...
mark IDK tests and batch tests as "nonblocking" so we don't wait
on them before doing builds. this risks running builds when the
IDK or batch tests could fail. It also unrisks spending an
extra five minutes in CI waiting for the last "test" phase thing
to run in a long dependency chain, which is potentially more
significant.
Make IDK tests run for changes to idk, client, or batch directories,
because at least one IDK thing uses the batch importer. Also,
make that actually work, we think -- verified that the IDK tests
got run when there were changes in that directory, but there
aren't any now, so we don't expect to see them.
Also, move the "IDK changed" rules into their own heading and
incorporate them by reference, and simplify the conditional because
it seemed to be Acting Up, but also make it check against
refs/heads/master, rather than possibly just the parent commit,
since that seems to be more consistent.
Finally, we combine four of the tests (go tests, go tests future,
go tests future plg, go tests shardwidth22) into two tests, both
using the "future" compiler, one for plg, one for shardwidth22,
so we don't need as many parallel runners and are less likely
to end up waiting on them.
At this point, the largest delay in CI is the chain through
building lattice, which blocks some tasks for a fairly long
time because they just have to wait for us to have built a
container we can use as the server container in tests that need
a server to work against.
SQL BULK INSERT
This change is to support a BULK INSERT/REPLACE statement that adds the ability to 1) take its input from a file, url or in-line blob 2) to map from the input source to the target columns
3) to transform data (using sql expressions) before inserting
4) support csv and ndjson formats
* improving test coverage
* increase test coverage again
* refactoring for handling transformation with types other than id and int
* Update package scripts so they restart services based on the operation (update vs install)
Co-authored-by: Julio Martinez <julio.martinez@logicmonitor.com>
For unrelated reasons, we renamed the protobuf interface from
package "pilosa" to package "proto". This means our GRPC endpoints
need to live under "proto.Pilosa" instead of "pilosa.Pilosa". This
means lattice needs to be configured the same way.
userInfo at the relevant line can be nil here. We check later if the
userInfo != nil (and assure it passes authn/authz) or if userInfo == nil
then we return all the indexes we can find.
This fixes the OriginalIP and RequestUserID in the main featurebase
package, and the Access and Refresh tokens, the UserInfo, and the
[]string of Indexes passed with context.Context(s) in the authn package.
An empty struct was used for all of these keys (and relevant helper
functions we added) to avoid allocations where possible while still
using the context functionality.
Some of the logic in the server.GetIndexes function was fixed.
This provides us with most of the existing Tx interface, split
across QueryRead and QueryWrite. The functions not included here
are the ones that are used *only* for anti-entropy (ForEach
and ForEachRange).
We add additional testing to verify that TxStores are getting
closed correctly, to go with cleaning up the test directories they're
made in.
We also introduce some test wrappers that can automatically
fail tests on error, so tests don't need to be full of error
checks.
Also, now that I'm starting to think more about the flow of
writing tests using QueryScope, we add the missing "full
database" scope option, and make the Add methods return
their operand so (1) you can chain them, (2) you can use
the AddIndex(...) inline in a NewWriteQueryContext.
Also addressed a plausible performance concern in shardList,
and some comments that were stale or incorrect.
The test coverage here is skimpy on the actual RBF-calling
functions because those are trivial. We do, however, significantly
expand coverage in the random write requests, which are now
a mix of random writes and random reads, and add test cases
that at least hit a lot of the error checks once.
The Error() method is changed to be like (testing.T).Error(),
taking ...interface{} and using fmt.Sprint on them.
There's also some minor tweaks such as making the visualizations
more consistent, testing visualization generation on two kinds
of keysplitter, and so on.
I was wondering why this is exported, and the answer is, if it
weren't exported, staticcheck would have reported that it was unused,
which it is. We don't need a wrapper on os.MkdirAll that we never
use.
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.
refactored comparison, equality and arithmetic expr eval for decimal data types and added a test to cover expression eval for inserts
fixed failing test
The internal/ingest and internal/schema endpoints were developed with
intent that they'd be the primary interface new users would work with,
because they were Easy To Use, and did not require any kind of setup,
the counterpoint being that ingest done this way had performance issues
because it ended up with huge amounts of JSON parsing to reformat
things into our native format. But this was understood to be the price
of providing a new-user-friendly JSON ingest experience.
A year later, we have no evidence that it's ever been used. We never
even moved it out of the `/internal` path. It's a lot of very complex
fiddly code and we don't seem to be using it, and at this point, our
anticipation is that if we really need something, we'll use CSV, which
we already have working, or something in the new SQL code. Either way,
we don't seem to be using this.
Apparently terraform can give us output saying that it succeded,
but give us an empty string for an IP address, which doesn't actually
let us use the IP address. Check for that case too in our overly
fancy setup.
This is a precursor to figuring out what's going wrong in a way
that lets us fix it more properly.
Also, request values, but don't instant-exit if they aren't present,
so we can actually do the retries.
We think etcd's tendency to mistakenly mark nodes down may have
been addressed. We can't find out without checking for it.
The exact pool of methods in methodsDegraded may have bitrotted
some; for instance, it didn't have PastQueries or PartitionNodes
in it, but it looks like it reasonably should.
We rework the Replica1/Replica2 server tests to reflect the
intended semantics again.
The special case of Starting allowed us to make sure every node in a
cluster waited for the whole cluster to come up, but caused problems
later if a node died and came back. We drop the Starting state for
clusters, treating a STARTING node as equivalent to an UNKNOWN (or
DOWN) node for purposes of cluster state, so clusters will go from
Down to Degraded to Normal as nodes come up. We now wait for the
Normal state during initial bringup. We would previously have accepted
Degraded, if you could reach it, for instance if a node came up and
then went down again before another node finished starting, but I'm
pretty sure that was unintentional.
This solves a problem where while a node was down, we'd accept
queries that we could handle in a degraded state, but then we'd
*stop* accepting them when the node started coming back up.
* Formatting adjustments made during code review.
While reviewing the BULK INSERT logic (in order to decide how best to
approach "ingest via sql" in the cloud), I made a few formatting and
comment changes. I'm just adding them here as a separate commit so they
don't muddy up my actual work.
* Parser modifications to support mulitple tuples in INSERT INTO
This commit doesn't include all of the changes required in the
planner. Fow now, the planner is simply modified to continue supporting
a single tuple (the first tuple in the list).
* Update the planner to handle multiple INSERT INTO tuples
This is part 1. It's still using the existing logic which builds an
ImportRequest for every record (and every field!).
The next step will involve using a client.Batch to handle the records.
* Introduce client.Importer interface (used by client.Batch)
Instead of the Batch having a pointer to a client, this puts an
interface there instead (which the client implements). It also allows us
to inject a different importer (i.e. other than a featurebase.client)
into the Batch.
* Decouple batch from client
This commit pulls batch-specific code out of the client package and into
a new batch package. It introduces the batch.Importer interface, the
methods of which replace all the calls that batch was previously making
directly to client methods.
Finally, it contains two implementations of the batch.Importer
interface: one is a wrapper around client, and the other is a wrapper
around featurebase.API.
* Use docker (instead of MustRunCluster) for internal batch tests
Because the `batch` package tests are internal, using
test.MustRunCluster() resulted in an import loop (because it eventually
imports `server`, and we can't have that). So this commit replaces the
use of `test.MustRunCluster()` with docker. The setup is basically the
same as that used in the idk docker tests.
Here we also remove all client-side references to `UseIngestAPI`, which
is an experimental (json) ingest api. It's still suppored on the server,
but here we remove the external usage of it.
* cherry-pick fix
* Use batch.Import() for sql3 INSERT INTO statements
* Thread logger into sql3
* fix batch test
* Fix some shadowing complaint by linter
* Address some test issues related to stringsets
* Exclude batch integration tests from CI
* Address PR feedback
- Added description to batch.README
- Consolidated grep commands in .gitlab-ci.yml
- Removed some debugging comments
- Replaces some inadvertantly removed license headers
* Add batch package to gitlab CI
* Updated CI for batch package
Updated CI include path
Update gitlab ci
Update CI
Update CI
Trying new include path for ci
Updated gitlab ci include path
Made idk race job optional for sonarcloud upload
add testdata directory
remove testenv from dockercompose file
use GIT_STRATEGY clone in batch CI
add testdata volume to dockercompose
Co-authored-by: Fletcher Haynes <fletcher.haynes@generalassemb.ly>
This is living in a subdirectory for now so we can have better
turnaround time on tests and not have to build everything else
along with it.
This covers the logic that we can have *without* actually using
databases or the filesystem in any way, just to provide a framework
that lets us validate the logic handling overlapping queries.
The overall purpose of this is to prevent deadlocks, by ensuring
that database locks are only taken when we have already proven
that they are available. In short, the QueryContext preregisters
its "scope" -- the set of things it may want to lock. The operation
of creating the QueryContext can block, but it blocks with no
database locks held. Once it is unblocked, the scope it has reported
is now considered unavailable, and no other QueryContext using any
overlapping scope can complete creation until this QueryContext
completes. While it's running, the QueryContext can't request write
access to anything outside its scope. Thus, once created, a
QueryContext can always proceed, without being blocked, until it's
done.
Note that this does not fully address multi-node behaviors;
once you have a QueryContext blocking things, you need to not
make queries to other nodes that could be blocked in turn by those
nodes. In short, no write queries to other nodes while holding a
write-type QueryContext on the local node, because if two nodes
do that to each other at once, they can both be blocked.
We believe RBF is currently designed such that read-only accesses
don't block progress on writes, so non-write access doesn't
create problems.
We also have some code to allow us to create dot-format output
from the components of this system, which is mostly intended to
be a debugging tool.
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.
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
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.
* 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
* 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>
*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.
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.
We end up spending a lot of time waiting for machines to become
available to run the multiple nearly-identical build phases. Instead,
let's just run one build phase that builds all four targets,
because the actual `go build` takes a tiny portion of the time
of the whole job.
We also drop the separate "pretest" phase which, while it was
intended to speed things up by getting that work done sooner,
actually just meant that all the other test phases were blocked
waiting on the build in a way they didn't need to be.
We also resume trying to skip the IDK tests when they're not needed.
We run sonarcloud after the external lookup tests, instead of
after clustertests, because clustertests are long and virtually
never fail, so this saves us a couple of minutes >95% of the
time at the expense of possibly running a useless test in the
rare case where the clustertests fail.
Redo the split of the various IDK builds (some of which need to
be done native on amd64, some on ARM) so that they are more similar
in length instead of being 3 minutes and 10 minutes.
Drop the non-auth variant of clustertests because it doesn't
really increase test coverage.
We fixed a number of performance issues as a result of which none
of the `go test` or `go test race` things should take more than 2-3
minutes, which means we definitely don't need to set a 90m timeout,
especially when the gitlab timeout is shorter.
All jobs now use GOVERSION or GOFUTURE to determine the docker image
pulled.
GOFUTURE is latest so that it will always use the latest version as new
versions are released. We can later lock it to a specific minor version
when there is another one released.
Co-authored-by: Garrison Davis <garrison.davis@featurebase.com>
* first try to skip decodeMessage() error
* force idk to skip a row if there are errors in recordizing
* add comment for removing returning errors from decodemessage()
* added ingest flag SkipBadRows and implementation for skipping Bad Rows (errors that come from recordizer)
* added some comments
* removed comment
* made changes as per discussion with jaffee and walter. i hope this works...
* adding unit tests to check functionality implemented for CLOUD-940
* addressing review comments
* changed a variable name in test file
* removed a variable from ingest test file
* testing sonarcloud failure
* drop spurious second sonar-scanner call
We call sonar-scanner on the IDK data, and then we change
into the IDK directory and try to run it again on the same files,
which don't exist.
* abandon idk change detection for now
the "changes" rule appears not to be good at detecting changes
in some cases. specifically, it appears that you have to be in
an "only:" clause, not a "rules" clause, to trigger the
merge-specific behavior which checks the entire merge branch
instead of the top commit, but that means that if your last
commit doesn't touch IDK, we don't run IDK tests, and I haven't
been able to fix this yet.
So for now, revert the IDK-specific change detection behavior,
which slows CI down but gets us test coverage.
* fix path references
we had three tests all creating idk_coverage.out, then we tried
to grab all files named coverage.out from the testdata directory.
* refactoring tests to avoid duplication
* reverting changes made for local testing
Co-authored-by: CHIN JUNG CHENG <chengcj@CHINs-MacBook-Pro.local>
Co-authored-by: Pranitha-malae <56414132+Pranitha-malae@users.noreply.github.com>
Co-authored-by: Pranitha-malae <pranitha453@gmail.com>
Co-authored-by: Seebs <seebs@molecula.com>
When we've started a fake cluster, we should expect to reach a
"STARTING" state, not a "DOWN" state. This test would coincidentally
pass as long as we checked the state before any of the nodes got
their notification from the node watcher that at least one node was
STARTING, because prior to that the cluster would be DOWN. But once
it got to STARTING, we would wait forever; we never reached the
instruction to tell the nodes to come to any other state, and they
would never reach a DOWN state.
the "changes" rule appears not to be good at detecting changes
in some cases. specifically, it appears that you have to be in
an "only:" clause, not a "rules" clause, to trigger the
merge-specific behavior which checks the entire merge branch
instead of the top commit, but that means that if your last
commit doesn't touch IDK, we don't run IDK tests, and I haven't
been able to fix this yet.
So for now, revert the IDK-specific change detection behavior,
which slows CI down but gets us test coverage.
We call sonar-scanner on the IDK data, and then we change
into the IDK directory and try to run it again on the same files,
which don't exist in that directory. We shouldn't be running it
twice; we should run it once on all the files.
More subtly, we created files named foo_coverage.out, then tried
to glob files named coverage*.out. (The apparent similarity of the
$(PROJECT)_coverage.out names is harmless, PROJECT is getting set
and they're using different names.)
Fixing this gets SonarCloud more reliable again.
We want to retry our terraform setup if it fails, so let's check whether
it worked and possibly retry.
This loop is awful because I'm trying to both check the exit status
and the reported IPs. Once I know whether the exit status predicts the
reported IPs that should go away.
* ID sql3 internal type representation is int64; fixed a bug that assumed incorrectly that it wasn't
* refactored some names for clarity
* primary: get nested loop joins to work; secondary get brute force aggregations for SUM working
* added tests; removed debug output
* review feedback
* Update sql3/planner/compileselect.go
review feedback
Co-authored-by: Travis Turner <travis@pilosa.com>
Co-authored-by: Travis Turner <travis@pilosa.com>
The testhook post-test hooks only work if you use a TestMain to
invoke them, otherwise the cleanups can be registered but never
actually get run. This deletes the etcd sockets, and temp
directories, that we created from our test runs. We also fix
the test creating a temp file directly to create it in a TempDir
(which gets cleaned up after the test), and fix the name of the
top-level tests displayed in TestMain.
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.
The default client appears to be pretty spammy and flood us with
debug messages about POST and GET requests, and honestly we don't really
need these or benefit from them, I don't think, so let's not.
A few view functions were taking a Tx, which had to be shard-specific,
but that's sort of awkward -- the view is inherently not shard-specific,
so it should be handling sharding internally.
There were also a couple of remaining obsolete checks for whether a
Tx was nil, at least two of which were in contexts where it absolutely
can't be. Remove all of them, and also the function itself.
* handle multi field count correctly
COUNT() should ignore null values.
If the data type of the expression supports an existence bitmap for the underlying FeatureBase data type we will use it to eliminate nulls from the aggregate
* simplify aggregate for existence test
we can use a direct != null instead of an indirect not(=null), and
avoid relying on the probably-broken behavior in the executor that
tries to silently fix up Row(x=3) tests on BSI fields which wanted
Row(x==3).
Co-authored-by: Seebs <seebs@molecula.com>
We had this fail in CI once, and failing took 30 minutes because
we didn't have a timeout on this. This shouldn't ever fail, but
the fact that it did indicates that the fabled etcd failures
we've seen a couple of times were still capable of happening.
This will make that failure happen sooner and more clearly.
Also, log the cluster states (and possibly node states) while
waiting. But add a delay -- otherwise we can do this quite a few
times per millisecond. We use Logf so that, if you didn't use -v,
you see these reported only if the test fails, but if the test fails,
we'll say what happened.
It would probably be better to have a passive thing that can wait
for updates, because we're waiting on heartbeats. Missing: A way to
detect what's actually happening in the failure cases, which we
see only quite rarely.
This is a bit complicated and entangled, sorry.
First, we squash the auth-based smoke tests into the regular smoke
tests; we just run all the tests with auth on and that way we don't
need to spin up an entire separate cluster of machines just to run
a single query against them.
We improve the error detection, and standardize the jq-to-get-config
code. The purpose of this is to try to make sure that, if we actually
hit a failure and get "null" for a host name, we report *that*
as an error, rather than running ahead and producing 20+ separate
reports that ssh failed because it couldn't find a host named null.
You have to start the cluster before you can refer to its holders,
because GetNode doesn't work on an unstarted cluster, but if you
actually issue any commands, those require messing with the worker
pool which wants to have access to the holder's stats.
"featurebase cli" will now save command history to
$HOME/.featurebase/cli_history by default. Additionally if a command
is entered across multiple lines, the newlines will be removed when
the command is saved in the history. Previously each line was saved
separately which was a bit annoying.
We were using v1.2.0 of the github.com/satori/go.uuid library to
generate UUIDs for transactions if the transaction had no previous id.
That version of the library had CVE-2021-3538: "Due to insecure
randomness in the g.rand.Read function the generated UUIDs are
predictable for an attacker."
More reading can be done here:
https://pkg.go.dev/vuln/GO-2022-0244https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3538
This vulnerability was found using the new govulncheck tool which is not
currently used in our CI pipeline but might be a good candidate to
include in the future. (Like all tools like this there are caveats to
its usage and utility which can be read about below.)
Information on that tool can be found here:
https://go.dev/blog/vulnhttps://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck
* first cut of working (slowly) bulk insert; table valued functions and a tuple data type to support time quantums
* oversight
* filter pushdown implementation; bulk insert
* addressed some linter issues
When doing tests, we create a ton of one-off clusters. This
turns out to be expensive and slow. Fixing it is surprisingly hard.
Fundamentally: If we're sharing clusters, we need to use different
indexes for each test, to avoid clashes. This changes index names.
As a side-effect, this reorders many partition-based things, like
the order keys are returned in. Thus, to fix this, we change a lot
of tests to no longer depend on the *order* in which strings are
returned.
Having done that, we can also discard the ModHasher behavior, since
that only existed to allow us to reliably predict partitioning.
The basic design is as follows: Instead of a cluster being a
[]*Command, a "shareable" cluster is now a []*Command plus some
flags, and a "cluster" is a pointer to a possibly-shared cluster,
plus a link to the specific test using this specific cluster,
and correspondingly, its test name suitably coerced to be a valid
index name prefix.
The "test.Cluster" object now has methods to allow retrieving an
index name, and also implemnts fmt.Formatter to let you use,
e.g., `%i` with it in Sprintf to get "the index name, plus an i".
(This works for everything but %p and %T.)
This allows us to consistently rework all the many things that
use index names in a persistent way.
We also have `MustUnshared` and `MustRunUnsharedCluster` methods
which allow us to specify that a given test needs its own cluster
for some reason. For instance, the tests that want to run backups
need their own isolated cluster, and the tests that want to close
or reopen nodes need their own cluster because a reopened cluster
won't have working GRPC for some reason.
On "closing" a shared cluster (actually the test-specific wrapper
that reflects a given sharing), we delete any indexes starting with
that test's index name prefix. Otherwise, the huge pile of open
indexes prevents `go test -race` from working on MacOS, where we
run out of address space too quickly.
This is fairly enormous but most of the individual changes are
fairly trivial things like replacing the string "i" with "c.Idx()".
We also tweaked a test that failed for me a couple of times to
not depend on sort order.
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.
For arbitrary mod values m, greater than zero,
(x%m + 1) != 0
is always true
What we almost certainly meant was
x%(m+1) == 0
which would give you all the bits in row 0, half the bits in row 1,
etcetera.
Also, we drop to doing a quarter-shard because why not.
This test used to be large, because it was testing some features that
were refactored out in October of 2019. Since we no longer have the
"buffer growth" to check, let's check a much smaller file.
We want to be able to register hooks which do cleanup, which may be
registered after the auditor cleanup check, which means that we
want LIFO order for post-hook cleanups.
We also want the test hook cleanup to be deferred, rather than
merely run after the tests are executed. Also, we have to extract
the result from running the test, then execute deferred things,
*then* call os.Exit, because os.Exit bypasses defers.
The "field/view will just synthesize a tx" behavior is awful and
also hides a number of fundamental flaws. We distinguish between
"we really do mean to work on a single shard here" and "we intend
to work on the whole field or view", and the latter now take
Qcx instead of Tx.
This eliminates a lot of very weird cases where we checked for
nil Tx and synthesized them, and also gets us away from
field and view taking Tx parameters when no possible Tx
can be constructed which is valid, because Tx are inherently
shard-specific at this time.
* squashed 45 commits into one :)
* tlt/sql experiment (#2035)
* Move PlanOperator to sql3/planner/types package
includes:
type PlanOperatorColumn struct
type PlanOperator interface
* Remove planner dependencies from pilosa package
The goal after this is to prevent the planner package (which doesn't exist yet)
from being imported by the pilosa package; we just want it injected into the server
in server/server.go. This is because the planner package uses pilosa types, so we need
to avoid circular dependencies.
Added ExecutionPlannerFn
Make public: pilosa.ExecOptions
Added a pilosa.Executor interface
Added a planner.types.CompilePlanner interface
Isolated the planner calls to:
- Executor.Execute()
- *API.[method]()
* Move executionplanner files into the sql3/planner package.
This required a bit of gymnastics, and there are some things around
FieldOptions which need to be addressed soon.
* Remove the hacky FieldOptions stuff I added earlier
This implementation just uses the pilosa.FieldOption functional options
provided by the API (as opposed to trying to build a FieldOptions
object.
It also changes field types to constants. These are private for now, but
if we need to make them public, we should put them in the planner/types
package.
* Implement the "scale" value from Decimal(scale)
Also, precision and scale were currently reversed in the parser. This
fixes that.
* Modify the parser to handle CACHETYPE <type> SIZE <size>
It's a little odd to me that the cache type values are Tokens, but I
guess it's ok. One thing to keep in mind is that FeatureBase expects
lowercase values, so this commit changes the parser to set the value to
the lowercase version of the type.
* Fix the /sql2 tests
This entailed a combination of commenting out or t.Skip()-ing tests
which covered code in the parser that has been commented out or removed
as not currently supported in sql3.
It also adds some coverage for the sql.Contraint stringers.
* Prevent JSON sql results from containing closing commas
This commit just re-works the existing output code to avoid inserting
closing commas (which results in invalid JSON).
* Enhance the CREATE TABLE test coverage.
In particular, ensure that the fields which get created in FeatureBase
are what we expect based on the fields defined in the CREATE TABLE
statement.
This also ensures that the TIMEQUANTUM and CACHETYPE contraints are not
provided for the same field (since those constraints are not supported
together).
* Adjust the EBNF file to indicate SIZE contraint is optional
A CACHETYPE can be provided without a SIZE. This change indicates that
SIZE is optional.
* Remove `executionplanner_` from file names (#2040)
* implementation of ALTER TABLE (sans column RENAME)
* refactored expression analysis; added more robust type checking; all unary and bin ops function on ints
* added type support for expressions; full bin/unary op support; added cast; more literal support
* cast int to all other types
* all literals (except idset, stringset & timestamp) make it thru; cast to all types with int as source now works
* implemented LIKE/NOT LIKE
* Implemented IS [NOT] NULL
* Move sql2 files into sql3/parser package (#2045)
* Move sql2 files into sql3/parser package
This also removes the sql2 package.
* Fix tests which were typing _id fields as INT intead of ID
* implemented BETWEEN, NOT BETWEEN
* Add featurebase/error package (#2046)
* Add featurebase/error package
I copied the `dax/errors` package which I am starting to use in the DAX
prototype into `featurebase/errors` in order to start using it with the
sql3 package. It's basically a wrapper around `github.com/pkg/errors`,
but it uses a customer coded error.
The sql package can define its own errors based on the
`featurebase/errors` types. Then do things like `Wrap()` and `Is()`.
* Address the linter complaints: shadowed variables, unreachable code
* implemented IN & NOT IN with expression lists
* first cut of CASE
* Fixed some errors from rebase
* updated bnf; removed unused code; tightened up error handling
* first crack at basic CLI for SQL3
Use: `featurebase cli`
Still lots to do here, but for example:
> select count(*) from tremor
+--------------+
| COUNT |
+--------------+
| 1.158321e+06 |
+--------------+
* Iterate on the CLI (#2057)
Handle the errors.
Add an "exit" command.
Add some general formatting and white space.
Add termination character: ";" (semicolon)
This commit allows a user to provide multiple or partial SQL statements.
Example of multiple statements:
```
show tables; select * from foo;
```
Example of partial (multi-line) statements:
```
select *
from foo;
```
Don't uppercase the header values
* error refactoring; first cut of TOP; remove unused code; use log.Printf instead of fmt.Printf
* fixed a bug with QualifiedRef from refactoring; added bones of INSERT; removal of unused code; tightened up errors more; fixed failing tests
* single value list for INSERT
* Update bnf per discussion with Travis; INSERT now doing the requisite stuff
* Pat's eyes went square - nothing wrong with TOP, Pat needed to learn arrays again.
* improved some errors; fixed tests to suit
* send warnings back in the api; update CLI to display warnings
* start warning on stuff not implemented so we don't get bugged about it
* Tlt/sql experiment (#2063)
* Expresssion -> Expression
* Add SQL planner test
- adds a test to which it is easier to add tables and SQL statments
- un-exports all of the expression types
- removes the planner pointer from the expression types (it can be added
back later if need be)
* Fix where clause on a string field
Prior to this commit, the binary expression for a where clause on a
string field was building the call by providing a range operator which
is typically used for BSI fields. This changes it to use the call.Args
for string values.
* Update planner tests to handle multiple sql for the same results
* Reorganize SQL tests
Introduce a test/helpers package and move shared MustQueryRows into that
package.
* Add a compatibility map for field types. (#2064)
This is primarily to address the fact that ID fields were previously
incompatible with INT literals.
We should probably consider introducing a custom type for FieldType
which can be used to define compatibilities.
* significantly refactored type checking
* Handle nil (NULL) values in the sql CLI. (#2067)
go-pretty panics if the interface{} field value is nil. This replaces
nil values with a "NULL" string.
* Squash some commits
fixed a still failing test
added line, col to all error messages
refactored source handling to enable table aliases
fixed some copypasta per review
warnings for order by & topn; implemented select as a source
starting to handle in (select...); added stub for optimizer
JSON-encode the sql error and warning strings (#2069)
Error strings with unencoded characters (like double quotes) were
resulting in invalid json.
got insert working; added symbol table; added concrete optimizer; added nascent NestedLoopsOperator; rewrite "where foo in (select..." as inner join
* all about the sets (#2085)
* implemented setcontains()
* implemented set literal; insert set column values; setcontains/all/any both in expr eval and pql filters
* Convert test to use latest framework. (#2086)
* fixed some comments
* removed refactored tests
Co-authored-by: Travis Turner <travis@pilosa.com>
* Add support for Decimal fields to the sql test. (#2090)
* dates (#2094)
* return dates as strings in output; tightened up decimal type checking
* return dates as strings in output; tightened up decimal type checking
* fixed failing tests after decimal changes
* can now insert decimal values
* implemented insert for timestamp data type; implemented current_date, current_timestamp constants
* fixed some failing tests
* handle date literals from strings in insert statements
* changes from feedback
* Fix pointer method error
* sql3 API interface (#2110)
* Introduce API-related interfaces: SchemaAPI, ComputeAPI
The sql3 code was relying on the pointer: *pilosa.API in order to call
API methods directly on the local node. If we want to import and use the
sql3 package in another service (the DAX queryer, for example), we need
to be able to use an implementation of an interface for those API method
calls.
This commit introduces two interfaces, both automatically implemented by
pilosa.API:
- SchemaAPI
- ComputeAPI
* Convert sql3 code to use IndexInfo instead of Index
The sql3 code was relying on a *pilosa.Index and its methods to get
general information like index and field name, type, etc. This commit
converts everything to use a *pilosa.IndexInfo instead.
This allows us to modify the SchemaAPI interface to also return
IndexInfo instead of Index, which will be a lot easier to implement in a
non-pilosa package (like DAX); creating a *pilosa.Index requires
providing things like data directory paths and holders, which are not
necessary for these use cases.
* Unary and Binary Ops R US plus CAST (#2111)
* implemented string literal for timestamp epoch
* fixed failing test
* fixed the failing test again
* refactored tests; implemented unary op tests for all datatypes; implemented binop tests for int/int, int/id, int/decimal & ID/int
* implemented all binary ops for INT & all other types, ID & all other types
* implemented binary ops for DECIMAL types & all other types
* added STRING & BOOL to various tests; implemented all remaining binOp tests
* fix up some stuff after rebasing
* refactored test defs into multiple files; implemented CAST for every datatype
* added tests for like/not like
* addressed review feedback
* addressed type review feedback
* tightened up IS [NOT] NULL behavior plus tests (#2118)
* tightened up IS [NOT] NULL behavior plus tests
* BETWEEN/NOT BETWEEN with all data types
* addressed review feedback
* Handle negative integers in column min/max constraints (#2120)
This commit parses the min/max contraint as an expression, as opposed to
an int literal, so that negative values are treated as Unary
expressions.
There currently isn't support for min/max constraints on `decimal`
fiels, so for now this change only expects +/- integer values.
* Implement the CREATE TABLE keypartitions logic (#2123)
* Execution time, IN/NOT IN & multiple aggregates (#2124)
* added display of execution time
* IN/NOT IN tests for all data types
* fixed date parsing
* removed duplicative tests
* refactoring aggregates
* suport multiple aggregates
* Address review feedback
* final round of feedback
* Add method SchemaAPI.CreateIndexAndFields() (#2127)
In order to support a CREATE TABLE statement as a single command, this
commit alters the SchemaAPI interface to contain a single method which
handles both the index and its fields. It also updates the sql3 code to
use this interface instead of CreateIndex() and CreateField()
indepedently.
* Symbol Handling (Again) (#2129)
* Refactored symbol handling in the planner; re-instated the select as source tests
* removed commented out code
* addressing review feedback
* Move hard-coded _id field out of planner and into interface implementation (#2130)
This commit moves the hard-coded addition of the `_id` field from the
planner to the SchemaAPI.IndexInfo() implementation method.
NOTE: If anything was expecting SchemaAPI.Schema() to also return the
`_id` field as part of its field list in each table, then it would not
be there because the `_id` field is only added in the IndexInfo() method
for now. Currently that's not a problem because nothing is expecting the
`_id` field for `Schema()`.
* Multiple aggregates, all aggregates stand alone and in GROUP BY (#2132)
* handle multiple aggregates in group by queries
* added handling for avg() aggregate both stand alone and in group by
* tightened up sum & avg outside of group by
* added min, max & percentile
* added warnings
* Make MaterializedRowSet implement the PlanOperator interface. (#2133)
This commit refactors the PQLMultiGroupByOperator to have a PlanOperator
as its output. Then, when it initializes, it sets up a
MaterializedRowSet and populates that with the values from the multiple
group by operations.
* added explicit min/max pql operators
* saved a file I forgot to save
* per review
* Un-indent some if/else nesting (#2136)
Co-authored-by: Travis Turner <travis@pilosa.com>
* Add optional `name` argument to test structs.
This commit adds the `name` argument to `tableTest` and `sqlTest` so
that a test can be optionally named. This allows a developer to more
easily run/identify a particular test by name.
* Inbuilt functions (redux) (#2141)
* set functions type parameter type checking
* implemented datepart
* include SQL3 type in SHOW COLUMNS output
* fixed select as source; failing SHOW COLUMNS test
* select in select list
* dump output columns; handle optimization for select list subqueries
* make it an error to return multiple rows for a select list subquery
* added description
* contants and test coverage for datepart function
* SQL3 Refactor-palooza (#2182)
* removed unneeded IsAggregate()
* first cut of working nested loops operator aka INNER JOIN
* remove selectListItemPlanExpression
* added some warnings
* all the tests are passing again!
* addressed some linter complaints
* added basic order by
* bug fixes; added 'or replace'/'replace' to insert
* for insert references should return appropriately
* added back ability to use subquery singleton expressions
* removed dead code; fixed test
* json-able plan, Schema() plus refactoring
* fixed dumb code
* add some tests for time quantum behavior
* Code cleanup during review. Also fixed INSERT to keyed table bug.
This commit contains a lot of minor adjustments made during code review.
It also contains a bug fix that was preventing INSERT into a keyed table
(i.e. _id type STRING) from working.
Co-authored-by: Travis Turner <travis@molecula.com>
* Fix expected min/max on timestamp column test (decimal field)
I don't know why this changed, but presumably something to do with
decimal related work that happened on master.
* Fix compile problem after rebase
* review feedback
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
Co-authored-by: Travis Turner <travis@pilosa.com>
Co-authored-by: Travis Turner <travis@molecula.com>
Co-authored-by: Fletcher Haynes <fletcher@capitalprawn.com>
Upgrade Go to 1.19
* Use go install to install statik for CI/CD
* Switch from stretch to buster for idk
The stretch release doesn't exist anymore for go 1.19 docker images.
buster is a newer version of Debian anyway (v10 vs v9)
Co-authored-by: Fletcher Haynes <fletcher.haynes@molecula.com>
So, nShards used to be 10. If you run a complete test, with go
test -race, and you have the sample input for the unrelated
TestImportMutexSampleData configured to use 64K bit density and 2K
rows, everything is fine. If you run a partial test, everything is
fine. If you run a complete test with -race, but you skip
TestImportMutexSampleData, or reduce either the bit density or the
row count, you get a very strange panic where the go panic handler
panics trying to report what happened so we don't get a valid stack
dump. On Macs. This is as much as I could debug it after about 6
hours. Since there's no special reason to think we need all 10
shards, and 9 still tests the behavior, we're leaving this one a
mystery.
The comment says "convert to each type and compare", but it
doesn't convert, it just compares the given container result to
three different forms of the same result. That's neat for testing
BitwiseEqual but doesn't actually give us more information, and
it takes nearly 3x as long.
Quick checks aren't super helpful, but they can test random stuff
a bit, so let's keep them, but make them smaller. Also, let's cram
the random selections into the first quarter of a shard so we see
more container updates to fewer containers because that's probably
more interesting.
don't add/remove two million bits one at a time. instead, insert
small array containers, make them larger, replace them with bitmaps,
and so on. In short, we still create containers and change their
sizes in interesting ways, but we do it thousands of bits at a time.
This reduces runtime with -race from around 10-20 minutes to a second
or so.
We drop the "PartialContiguous" test because it was actually just
identical to PartialNoncontiguous, so far as I can tell, and not
really interesting.
We make clients pass in a context which has an associated
data structure which can be used to annotate errors we've encountered
inside retryablehttp. You might ask why we do this instead of
just giving retryablehttp a logger that isn't nopLogger; the answer
is that retryablehttp's logging is too spammy to use.
So we create a context, and then pass that in for requests, so that
when we check to see whether we should be retrying, we can log
the errors we encounter, so if we encounter errors we can log
something specific instead of "gave up after 5 retries".
We also make a second retryable client that would forward
authentication, and reuse that, rather than making a new client
for every call that needs to forward authentication. This allows
us to avoid the problem where that inner client wasn't picking
up the timeout settings we'd applied to the parent.
We log errors with Errorf if we actually failed entirely to do the
request, we log them with Infof if there's errors but the request
ultimately succeeds. We also try to log part of the response body
when there is one, but there wasn't an error.
If there's no response, we produce a spurious 500 and a message
saying that we produced it.
The IDK tests should be run only when there's changes in the IDK
or client directories.
Also the shard transactional test should be run ever.
The "optional: true" flag is a fascinating quirk allowing you to
express that, *if* a job exists, we should wait for it, but if the
job doesn't exist, that's also acceptable.
When we make dummy test servers, we should make them using sockets
for etcd rather than TCP ports so we don't run into problems like
the test always failing if anything else is on that port already,
which it can totally legitimately be. For instance, if you ran
an existing featurebase server, and then tried "go test" in the
server directory, this would fail.
We want "make test" to run in a reasonable amount of time and
actually work, the IDK tests are full of tests that only run in
a specialized docker environment with things like hosts named
pilosa and kafka and such.
* FB-1251: Add ability to sort Extract queries by some field
There is a sort call which takes a row call and the field, and
based on the field type, the corresponding rows are read. both key and
value are stored in RowKV{}. The value is stored since its required to
merge data from shards. values are sorted in each shard and these sorted
listes are merged in the reduce.
sort-desc flag is sent to comparator to decide the sorting order. ok
flag is added to the compare function to track any error in the
sort.Slice anonymous function
Sorting over set field was removed, since there would be multiple values
for each ids and there would be no right sorting order there.
Before, we were building docker images for IDK for each of the four
linux/darwin amd64/arm64 platfrom/arch combinations, which didn't make
sense. If we want to later build docker images for linux/arm64, we can
add that later.
I also cleaned up the Dockerfile for IDK to minimize creation of excess
layers (by &&-ing RUN commands), and made apt quieter to cut back some
of the noise.
We need to install base gosec tool and then the gitlab version to convert the gosec
json to the gl-sast-report.json that GitLab expects.
This lets us see the 'Security' tab under pipelines (and under the default branch
after this change is merged).
I chose to pin both of the versions of the tools to avoid any dependencies changing.
This could be an issue, but both repos are largely frozen.
We're using this because it's a builtin env variable that comes
with GitLab, and it fixes one of the annoying things about Docker
tags (e.g., you can't use all of the allowed characters in
Git branches).
One issue that I've seen a few times, is branches with either
capital letters (which was recently broken), or using the '/'
character.
This PR makes it so we always use the CI_COMMIT_REF_SLUG
when making or referencing images so that it is always consistent.
Note: this might make it slightly harder to intuit what the correct
Docker image to make (if you wanted to use the one built by CI rather
than locally). This trade off doesn't seem too hard to overcome.
There are currently three copies of a package called `fakeidp` in the
featurebase repo:
- ./idk/fakeidp/go.mod
- ./internal/clustertests/fakeidp/go.mod
- ./qa/fakeidp/go.mod
All three have a `go.mod` file. While this is supported under golang's
new Workspace support, what's not supported is that the modules share
the same name (in this case "fakeidp"). This commit is a sort of
temporary fix which renames the module for two of the instances. This
prevents, for example, VSCode with workspace support enabled, from
barfing.
By the way, one can enable VSCode workspace support with the following
setting:
```
// gopls
"gopls": {
"build.experimentalWorkspaceModule": true
}
```
Also...
This commit fixes the `make testv` target. It's probably not used
anywhere (which I'm assuming because it was broken), but it's a handle
target, so now it will list and run tests against all packages found in
the repo, including the root package.
* [CLOUD-934] Optionally broadcast IDK Kinesis errors/panics to external storage
- Add a minor public method `idk.Main.SetLog` to allow setting the logger instance
after initialization.
- Add a Logger implementation that captures recoverable errors and panics
and pushes to an external store. Meant to decorate an existing Logger
instance and always delegate to its implementation. Decoration happens
when all AWS resources are initialized. Before then, the wrapped Logger
implementation is used.
- If `--error-queue-name/CONSUMER_ERROR_QUEUE_NAME` specified, use an
ErrorStreamLogger to push errors and panics to an SQS queue with that name.
Omission of the option preserves current behavior.
- Parse sink ID from the `--stream-name/CONSUMER_STREAM_NAME` expecting the form
'PREFIX'-VALID_UUID. If the sink UUID is invalid, emit a warning that errors/panics
will not be written to an SQS queue but will still be logged using the decorated
Logger instance.
- The inability to push to an SQS queue leads to warnings being emitted to notify
ECS that no queue will be written to and is NOT a hard error.
- Add SQS interface mock for unit testing.
- Add IDK make targets for generating mock interfaces.
* [CLOUD-934] Execute go mod tidy and go fmt to pass CI/CD checks
* [CLOUD-934] Remove extraneous Makefile in idk/kinesis and fix install-mock-generator target
* [CLOUD-934] Add godocs to exported types and functions
* [CLOUD-934] Changed warning to not sound so ominous and update associated unit test
* [CLOUD-934] Unblock CI/CD at the IDK test stage
previously we allowed users to specify a granularity for timestamp
e.g. seconds, milli, micro, nano
however we converted everything to nano before we stored it.
This reduced the allowed range for all time units to what
was allowed by timestamp. For example, with second granularity
you can represent billions of years within the capacity of
int64 but with nano its somewhere b/w 100-200 years.
So now, for timeunits of seconds, milli, and micro the range
is year 0001 - 9999. These limits come from what Go
supports.
So this uses unit specific function to translate
timestamps to values and vice versa to increase
the time range.
In the process of increasing the range for timestamp and subsequent
testing, I found and addressed a few bugs:
- min/max queries were not using timestamp specific comparators so
added that.
- Values from Import/ingest come to FB as relative values to epoch
whereas other BSI fields come as actual values and then
becomes relative to their respective bases within FB. so some
specific handling of that was added.
- However! Set queries use timestamp strings which are, of course,
the actual value they designate. So they have to become
relative.
- When bitdepth is 0, Min/maxUnsigned functions did not run
resulting in a count of 0 when there
was an actual value that was 0.
Also, this removes (now) dead code and updates/adds tests.
most types are imported in the format `<row>,<col>`, but ints and decimals
aren't. with this new flag, ints and decimals are imported using the
`<row>,<col>` format, instead of `<col>,<row>`.
compares free space in output directory to
the usage of either the data directory or
index depending on what is being backed up.
- adds an http_handler endpoint to get usage
of a particular index
- adds InternalClient methods to get DiskUsage and
IndexUsage
* unifying idk and featurebase: first pass
* resolved conflict with master for gitignore & dockerignore
* deleted binaries that were accidentally pushed to git
* combined gitlab jobs for idk & featurebase
* run go fmt for idk
* updated ssh env variable, and made docker password variable in gitlab env variables
* fixed typo assigning variable name
* trying to fix docker login error
* trying a different solution for docker password
* pass registry
* fixed docker login
* updated paths for idk
* exclude idk tests from featurebase test run
* fix vendor error
* update certificates
* grpc needs to be in version 1.38
genproto, which is imported by big query updates the grpc version to 1.47.0
grpc 1.47.0 causes etcd to deadlock when calling etcd.Close()
the fix is to have a replace in go.mod to specify a specific grpc version
* run go mod tidy
* go mod
* run go mod tidy
* exclude bigquery since it is causing issues and undo grpc replace in go.mod
* fix grpc version
* fix formatting error
* update formatting
* attempt to fix formatting
* update path for code coverage
* update to use current branch binaries, not master
* fix for building idk - path updates
* udpate path for binaries
* update job dependecies
* update docker idk tests to use the current branch registry
* update stages for jobs
* updated job dependencies
* not allow idk s3 dump to fail since it is a dependency for integration tests
* update dependecy for idk tests
* update paths for idk build and code coverage
* download featurebase binary from s3
* pass branch name to all setup scripts
* change to current branch instead of master
* updated sonarcloud
* sonarcloud fix and branch name fix
* trying to speed up pipeline run time
* update stage
* branch name fix + sonar cloud
* sonarcloud
* Fixed broken fields when packaging rpm and deb files.
* Update systemd unit files, package them into RPM's.
* Fix config file path for packages.
* Changed unit and binary paths to conform to standard locations for each vendor.
* Added featurebase owned directories.
* Create featrebase user/group and chown the right dirs
* Automate turning on featurebase
* Updated the .gitignore to include .vscode files.
* Changed RPM name to better conform to naming standards.
* Pass GOARCH when building RPM's.
* Avoid using recursive to remove files in this dir.
In the logs, I can see that this error occurs when a query is done
during the delete view.This fix is only to bypass it and log that
data.
The real issue is that the delete standard view which should happen
only once, is occuring every hour or two. The ingester might be
creating the standard views which needs to be fixed.
The root problem this is attempting to address is sporadic
weird cases in which etcd mistakenly thinks it's down even when
it's up. I am not confident that this is addressed, but there's
a reasonable chance that it is, and I can't trigger it at the
moment, but it was always sporadic, so that doesn't prove much.
There's a lot going on here, and it comes into roughly three
categories.
First: Dropping unused/unneeded code. There's a lot of leftover
bits from the initial development and refactoring of this.
Second: Unifying and shuffling some of the design. We had
multiple interfaces which are functionally impossible to
usefully implement separately, so they're combined together,
and in some cases, moved.
Third: Streamlining logic and simplifying design choices.
This is combined into one commit because the changes are
thoroughly entertwined with each other and you can't usefully
break most of them out.
Also, a bunch of test coverage for most of these changes.
Big changes:
We merge the topology and disco packages. The topology and disco
packages being separate creates a complicated tangle of problems
and dependencies. The fundamental problem, approximately, is that
topology.Node has to track disco.NodeState.
There's three core interfaces interacting here:
topology.Noder (maintains list of nodes)
disco.Stator (maintains the state of a node)
disco.Metadator (stores, possibly retrieves, node metadata)
But the node state mantained by the Noder *is* the set of node
metadata, plus state updates produced by Stators. The only actual
non-trivial and usable implementation of these interfaces is a single
thing which implements all three, and in which the implementations
share a single backend data source which they are all modifying.
But you can't move Noder into disco, because Noder has to refer
to topology.Node, but topology.Node refers to disco.
Solution: First, merge these two packages. Second, merge these
three interfaces, to provide a single interface which is more
clear about the fact that (metadator.)SetMetadata() and
(stator.)Started() are both changing the output we'll get from
(noder.)Nodes().
We rework the node state tracking.
We have this nodeStates map which is almost unused. Really, we
don't need it at all. Every node's state is either its last heartbeat
state or "Unknown", so we simplify this a bit. Also, we ensure that
the populateNodeStates function itself is yielding the sorted nodes
list, so we don't have to be as worried about possible later lookups
of sortedNodes happening outside a lock. We also add diagnostics
for deleting nodes from the metadata list (this should never happen),
and try to track heartbeat state more closely.
This is *probably* what fixes the underlying reported problem,
if anything did.
Still an open issue: Make heartbeat state changes aware of when
they're talking about *this* node and possibly not try to
mark it down? Except this may have a flaw: That would result in
each node disagreeing with other nodes in etcd about the state
of that node in the failure cases, and undermine the point of
using etcd to keep these states consistent.
We reduce the number of contexts and cancelfuncs in the etcd wrapper.
We create a shared context for the non-etcd.embed children of our
etcd wrapper, the heartbeat/keepalive and the node watcher, so we
can cancel that one context and cancel all of those at once, so
we don't need to separately track a function to call to cancel
the watch, AND be closing another channel. Also, our shutdown
now propagates automatically to the various etcd API calls we've
made for things like the node watcher and keepalive calls.
We still need to watch that channel in watchNodesOnce, though,
because apparently the watch doesn't yield an error even if the
context calling it is canceled. Whee.
This should reduce the risk of ending up in an inconsistent state,
and also the Close() function is probably idempotent now.
Smaller changes:
* Remove config-generators that existed to generate etcd
configs but were used only for tests that no longer exist
or make sense.
* Move the logic to generate etcd configs into the etcd
package, instead of the "testing" subpackage. This allows
us to write a self-contained config generator for
clusters where the nodes know about each other, but do
this just with etcd, not with full featurebase servers.
* Move the thing generating `fake:%d` socket names into
the etcd package, which is the only place we use it.
Also simplify it slightly.
* Don't panic on invalid URLs, report errors from them.
* At least try to use etcd's config.Validate functionality.
It's underdocumented, so we're not sure what it will report,
but at least if it does we'll get reports from it and
know what they are?
* Try to handle CompactRevision errors from watches more
correctly -- after a CompactRevision, any future attempt
to watch from a lower revision will necessarily fail, so
we adjust our target revision up. We don't have good
testing for this.
* Drop the Metadata() method (that used to be in Metadator)
because nothing ever used it and it didn't make much sense
to try.
* Convert SetMetadata from taking an arbitrary json blob
to taking the only data that would ever be valid since
we always use it to extract node information anyway.
* Drop several unused functions, unexport things only used
internally.
* Replace Started() with SetState("STARTED"), allowing us
to write tests that mess with states. We weren't really thinking
carefully about state transitions sometimes and now it's much
easier to do that thinking.
* Stop leaving stray localhost:2380 and localhost:2379 in
our embed config. We still sometimes see peer requests from
those and I honestly don't know why, but at least it should
be rarer.
Changes to row queries with from/to options return an error if field is not a timestamp field
logic couldn't be made earlier in call stack because other queries process from/to time differently.
This PR reverts the timestamp work.
The timestamp work requires changes to FB and IDK; and there are
circular dependencies between tests in either repo preventing merging
of either. The work here is pretty stable, but required bypassing
the smoketest. Meanwhile, I found some additional things in IDK that
need addressing which means I merged this work in pre-maturely. Once
I get that worked out, I'll re-commit these commits.
This reverts following commits related to timestamp work:
bypass of smoke test b/c of circular dep with IDK: 0676790
update codec to reflect changes to timstamp range: bd5dc76
fix few bugs regarding timestamp: 33fce8a
increase time range for timestamp by using specified granularity: 5939923.
In the process of increasing the range for timestamp and subsequent
testing, I found and addressed a few bugs:
- min/max queries were not using timestamp specific comparators so
added that.
- Values from Import/ingest come to FB as relative values to epoch
whereas other BSI fields come as actual values and then
becomes relative to their respective bases within FB. so some
specific handling of that was added.
- However! Set queries use timestamp strings which are, of course,
the actual value they designate. So they have to become
relative.
- When bitdepth is 0, Min/maxUnsigned functions did not run
resulting in a count of 0 when there
was an actual value that was 0.
Also, this removes (now) dead code and updates/adds tests.
previously we allowed users to specify a granularity for timestamp
e.g. seconds, milli, micro, nano
however we converted everything to nano before we stored it.
This reduced the allowed range for all time units to what
was allowed by timestamp. For example, with second granularity
you can represent billions of years within the capacity of
int64 but with nano its somewhere b/w 100-200 years.
So now, for timeunits of seconds, milli, and micro the range
is year 0001 - 9999. These limits come from what Go
supports.
So this uses unit specific function to translate
timestamps to values and vice versa to increase
the time range.
We catch some possible states that don't make sense or are insecure:
1. If we're passed a nil tlsConfig to parse, return an error so we don't panic.
2. If we have a root CA, but we're skipping server cert verification, return an error.
3. If we have a TLS cert, but we're skipping server cert verification, return an error.
This way we can't get into an inconsistent state.
Consider this example:
You have a 3-node cluster, nodes A, B and C.
You create an index "blah" while all three nodes are up.
Nodes B and C go down.
You attempt to delete the index. It is removed from node A's holder, but is not removed from nodes B and C.
When nodes B and C are restarted, the schema still shows this "blah" index.
If you attempt to delete the index from node A, you receive an index not found error, but the schema indicates the index exists.
With this change however, when you first attempt to delete the index, it is not removed from the holder until there is enough nodes up to achieve consensus.
The same situation applies to fields and views.
We had two different, incompatible-with-each-other, and both
individually broken, partial implementations of resizing logic.
There's the original pre-etcd resize, and then the etcd resize,
and neither works, but there's conflicts between the ways they
don't work.
No attempt to fix this is likely to yield decent results, so
instead, we yank them both out entirely, so if we decide to
implement resizing (which we will) we won't be confused by
stray code pertaining to resizing that's not really hooked
up to anything.
We're leaving the resize messages in protobuf to avoid renumbering
protobuf messages. We rename some of our message types to UNUSED0,
etcetera, so that any code still using the old names won't
compile, to make sure we get rid of it, but we can't just drop
the numbers without breaking rolling restart.
The Resize_AddNode tests are removed not just because we don't
have resizing, but because they were completely broken anyway
and never worked at all. But there's no reason to fix them because
they exist to fix the functionality we didn't have and are now
removing the vestigial remains of.
We also drop the one usage of the AddNode function of Noder, because
it was used only by one test code fragment that was creatincg clusters,
and that can be done more correctly. There were no other call sites
at all.
We mark the monitorAntiEntropy function to be ignored by
code coverage because it's not actually being covered. There's
a separate ticket for removing that entirely.
Bug: if a sql query had a where clause within parens, the entire
clause would be ignored; and instead of it translating to a pql
intersection, it would become an All().
This occured b/c the parser library mapped such an expresstion to
a sqlparser.ParenExpr, and we did not have this as a condition in
a type switch.
So instead of treating a ParenExpr as nothing, we now recurse into
it.
The sonarcloud job was accidentally altered to use *only* the PLG
coverage data, which is incomplete for reasons not yet fully
understood. Unfortunately, it wasn't *waiting* for the PLG coverage
data to be complete -- the job could start before the PLG coverage
ran, which mean that you could get anywhere from a few percent
to nearly total code coverage.
Also, we want to be sure to cover *both* the PLG and non-PLG coverage
data, so we add the non-PLG coverage data.
We also factor out the simulacraData package from our PKG_LIST because
it appears to be confusing sonarcloud because that package isn't
"included in project" or something.
Also remove a stray `ls` that was probably part of the original
testing/debugging of this.
* Revert "make pql.Decimal.Value a private big.Int field"
This reverts commit eab6174388.
* Revert "pql.Decimal for DecimalVal in ValCount&GroupCount"
This reverts commit a0c9eec410.
* Revert "Add AddDecimal support to pql"
This reverts commit 50787fd37a.
This is fairly experimental, but basically, we make a fragment-level
op which, given a []uint64, can produce a union of all those rows in
the fragment, with a single scan through the fragment and not needing
a ton of additional space to reify all the rows at once.
Now with the Repair calls happening in the Results assembly rather than
on the intermediate data.
when running a select statement with an inner join where the secondary field is non-existent, we get a panic. this commit fixes that.
see [fb-766](https://molecula.atlassian.net/browse/FB-766) for more information.
this was some quick work I did in response to a possible issue that
was reported. It didn't turn out to be a problem on our end and these
tests confirmed that, but I think this is worth checking in.
* automate builds of single node featurebase for PLG
* make plg target uses go build instead of go install
* corrected artifact names in plg build stages
* add s3 dump for plg
* edits to s3 dump for plg
We've seen this happen with relatively large datasets with a relatively low
max-file-size. The solution we came up with was to increase the max-file-size
config option, which works, but we still don't want there to be a panic if we
hit this again.
see https://molecula.atlassian.net/browse/FB-1381 for more information.
* create getter for monitor state
* refactor monitor
* fix http middleware
* change warn to error if attmpt to cluster on plg
* sentry: special considerations if execution is part of test
- skip test if they build a cluster as this will error by design
- skip sending messages to sentry if testing
* got single node working; refining error messaging and version info to follow
* better implementation that separates the build condition into etcd/enterprise_cluster.go and etcd/plg_cluster.go. go build will default to a clustering version and 'go build -tags plg' will build the non-clustering version
* added Makefile target for 'make plg'
* additional comments, CI/CD update
Co-authored-by: Kasey Rodgers <kaseyrodgers@Kaseys-MBP.attlocal.net>
We only actually check presence/absence in this map, we never
set the value stored to false, and we assume in some places that
any value present is equivalent to true, so we might as well
use a map of struct{} and save the several whole bytes of memory.
add sentry for error monitoring and performance tracking. Must call the init function to actually turn on the feature. This is expected to be used in the PLG binary and not the enterprise binary.
This ensures that we can't overflow when adding `pql.Decimal`s together. The
only place we can possibly overflow is when converting pql.Decimal to an Int64,
but that is a risk we have to take. Also, the only place we do this is in our
ToRowser. We could maybe change that to strings, so the presentation of data
doesn't indicate an overflow, but that is a later decision to make. It will
also involve fixing the generate-proto-grpc make command, because that's broken
rn.
This way we can avoid annoying floating point rounding errors.
Check out FB-1359 for an example:
```
--- FAIL: TestExecutor_GroupByStrings (0.55s)
--- FAIL: TestExecutor_GroupByStrings/3 (0.00s)
executor_test.go:5433: unexpected result at 0:
got:{Group:[generals.1.r1] Count:5 Agg:2775
DecimalAgg:27.749999999999996}
want:{Group:[generals.1.r1] Count:5 Agg:2775 DecimalAgg:27.75}
```
* mutex clear on nil support with test
* Update client/batch.go
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
Co-authored-by: Matthew Jaffee <jaffee@pilosa.com>
* spot instance test
* update outputs.tf to provide spot instances
* uptate outputs.tf data_node_ips
* propogate spot instance request tags to the instances
it's not something that we can send to the server (it calculates base
off of min and max), and we don't need it in the json string in order
to use it client side when building import requests.
There are 16 containers in a row in a shard with the default shard
width of 2^20, but since you can change the shard width at compile
time, everything should be computed off that.
if the same ID is added multiple times with different values, only the
last value should get set. Without this change, if the multiple
records weren't immediately next to each other, all the bits from all
the values would get set.
the ranked cache must be fully rebuilt as the BitmapRewriter doesn't
have an easy way to track which rows had how many bits changed which
we would need to update the cache.
We also needed to add a Clear method to topn cache to completely
remove old values before the rebuild as otherwise they'd sit there and
pollute the cache after it was rebuilt.
This also includes fixing a strange idiosyncrasy where the _exists
field was a set field, but didn't have its type explicitly set. This
was causing it to have a ranked cache even though that option was
turned off. Hoping this doesn't have any weird follow-on effects... or
if it does the tests catch them.
This adds a shard-based import endpoint which takes bitmap data for
all field types and imports data for the whole shard transactionally.
It uses the BitmapRewriter interface to try to intelligently allow for
setting and clearing bits simultaneously without multiple writes which
is especially helpful when ingesting into int-like fields, but also
allows clear-and-then-set behavior for set fields.
* Add CleanOAuthConfig endpoint
We will use this to get the OAuthConfig information, without the client secret, from
FeatureBase without having to have access to the config file. This will be useful
for the auth-token subcommand.
* Add string manipulation utility functions
Go doesn't have native support for these kind of things, so I added this to make it
easier to do string reversal, and replacing the first string encountered from the
end of the string to the front.
* Add auth-token subcommand
This is for work on [FB-1379](https://molecula.atlassian.net/browse/FB-1379).
We need this new auth-token subcommand to allow users to get access and refresh
tokens without having to login to featurebase via the UI. This commit adds that
functionality.
* error on oauth endpoint if auth isn't on
* https as default scheme in cmd, not internalclient
* ugly first cut at supportings Rows(in=[...])
need tests, better handling of various combinations of arguments and
error cases
* explicitly error when other arguments passed with 'in' to Rows
* first cut at supporting Rows(in=[...])
'in' is explicitly not supported with any other arguments (except the
field of course), and will error. It works both as a standalone Rows
call and in GroupBy.
* bitmapfilter require ordered rowids
* remove log message
Co-authored-by: Todd Gruben <todd@molecula.com>
This fixes a bug where the frontend UI sends GRPC cookies via a single semi-colon
separated string, which our new token parsing algorithm did not recognize as valid.
Now we account for that special case, at the small expense of greater overall
computational complexity.
* FB-1403 - fixed TTL to not allowed negative values
* FB-1403 - TTL - fixed comment
* FB-1403 - TTL - added comments for unit test, removed extra if statement, removed extra unit test
* Add refresh token header/cookie
As part of work on automatic refreshing of access tokens in the grafana plugin
(FB-1377), we will now accept a refresh token in the "X-Molecula-Refresh-Token"
header or the "refresh-molecula-chip" cookie.
This refresh token will be used if the access token is expired. To achieve this,
there was a lot of plumbing that had to be done. Here is a list of some of it:
* Added lots of constants for the new values.
* Removed token cache, since we will be keeping state on the clients.
* We now only refresh tokens when they are expired, which is more inline with the
OAuth spec.
* Refactored SetGRPCMetadata to be simpler to read.
* Refactored AddAuthToken.
* Update failing tests.
* We now don't split GRPC cookies on ";". Not sure why we did that before tbh.
I also added TODOs to add the refresh token to other subcommands. This is out of
scope for my current ticket, but it would be nice to have in the future.
* remove unnecessary context from Authenticate
* Add comments on why we check both cases for headers
It's because some GRPC clients lowercase metadata names. I've run into issues with
this enough that I think it's worth the extra checks. We prefer lowercase though,
because that's "standard".
* Fix test that broke during rebase
* Use IP whitelisting for ingest
For ingest, use configured IPs to authenticate the requests.
Auth-token will no longer be used for requests from ingest consumers.
If IP in request is in configured IPs, authenticate and authorize as an admin.
If IP in request is not in configured IPs, proceed with the standard authentication/authorization using ADD.
* need to remove port from client IP
* addressed review comments
swallowing this error was a mistake... caller would not know that
ingest failed and might incorrectly report success
upstream (e.g. committing offsets to Kafka)
* Changed several gauntlet tests to be manual due to cost emergency
* Made perf_able manual
Co-authored-by: Fletcher Haynes <fletcher.haynes@molecula.com>
out of range. Previously internal server error was returned.
This is to allow for ingest to continue while logging bad values
instead of stopping ingest as we do when there is a server error.
We've been seeing weird retention of Tx that shouldn't still be open, and
one possible explanation is that, until a Tx actually uses the freelist
cursor (either to allocate a page or to release it back to the freelist),
the freelistCursor statically stored in the Db object continues to have a
pointer to the previous Tx which used it, which allows a Tx, and thus its
dirty page map, to be retained forever.
I previously thought this should also nil out the page maps in the Tx, but
the more I think about it, the less I think that's a good idea. The actual
lifespan of a committed Tx should be quite short. If it *does* stick around,
it's beneficial to us as debuggers to see those large maps of dirty pages
sticking around. So after thinking about it a lot I decided not to do
that.
Similarly, when closing out a container filter (whether a filter or
a rewriter), zero out the Cursor, Tx, and filter and rewriter functions.
(We don't have to worry about the cursor's Tx, because the cursor gets
closed, which zeros its Tx and returns the cursor to the cursor pool,
too.) This likely matters a lot less, as the filters in the pool
get garbage collected "soon", but it still reduces the amount of
stuff being retained.
* make CookieName an exported constant
* fix SetGRPCMetadata
this will actually set the grpc metadata even if there are no cookies in the
metadata already.
* gofmt yourself
* FB- 1456 - TTL - fixed views not returning correct results when least precise quantum are deleted
* FB-1456 - TTL - PR - fixed comment
* FB-1456 - TTL - changed getQuantum to getLowestGranularityQuantum since we only care about the least precise quantum that is available
* FB-1456 - TTL - removed unit test used for debug
* FB-1456 - TTL - fixed comments
* [FB-1435] BSI Base Fix (#2056)
* add bsi base back to int value
* test bsi base/min/max for IntFields
motivated by bsi base not being added back to values
in extract calls when min was a positive integer.
* FB-1456 - TTL - fixed comments
Co-authored-by: Samir Patel <48686912+54mir@users.noreply.github.com>
* add bsi base back to int value
* test bsi base/min/max for IntFields
motivated by bsi base not being added back to values
in extract calls when min was a positive integer.
* use mainline etcd-io dependencies, not forks
this commit does lots of things around clustering with goal of increasing stability.
- upgrades from molecula/etcd to go.etcd.io/etcd@v3.5.4
- upgrades from seebs/bbolt to go.etcd.io/bbolt@v1.3.6
- update tests to use unix sockets for etcd cluster communication
- this is what etcd uses for a lot of internal testing, so if their devs think
it's a valid test, we can probably accept that
- cleanup etcd node-watcher shutdown process
Co-authored-by: tgruben <tgruben@gmail.com>
* moved random query to another repo
it had weird dependency issues with upgrading to mainline etcd bc of the vegeta dep
so we removed it bc no one really uses it anyway
we got this error message:
```
github.com/molecula/featurebase/v3/cmd/random-query imports
github.com/tsenart/vegeta/v12/lib tested by
github.com/tsenart/vegeta/v12/lib.test imports
github.com/streadway/quantile tested by
github.com/streadway/quantile.test imports
.: "." is relative, but relative import paths are not supported in module mode
```
* add cleanup to EtcdUnixSocket test util
Co-authored-by: reesporte <reesedporter@gmail.com>
- terraform scripts to set up cluster
- cloud-formation scripts to set up cluster
- set up ingest node with kafka server and datagen
- set up second ingest node with molecula-consumer-kafka-static
- set up datadog in all nodes (ingest + featurebase)
- set up script to execute different queries
- only run delete test on schedule
previously, we would use the standard view if the query seemed to
cover all the views we had, or if we didn't seem to have any time
views. This is unintuitive if some views have been deleted (which
comes up a lot more often with TTL!). It's also unintuitive if you
know you haven't set any data w/ a timestamp and your query that
specifies a time range returns any data.
we have an issue where nodes get into UNKNOWN state rather frequently
during periods of high load when they aren't actually down. We want to
allow queries in this situation rather than giving a "shard
unavailable" message.
We default to the jmp-hash method which we had previously, and allow a
user to set the "modulus" option which uses a simple mod operation to
ensure an even spread of partitions across nodes.
I think that ideally we would have new indexes uses modulus and
existing indexes use jmp-hash which implies supporting this
configuration on a per-index basis.
If we don't do per index, we should probably run the whole test suite
both ways.
we change this to use a simple modulus to ensure maximally even
assignment of partitions to nodes rather than the hash thing we were
doing previously which may have helped minimize data movement when
adding nodes, though I'm not even sure of that.
The logic was duplicated in a few places, so we've also condensed
that. For now, we're skipping tests which have baked in assumptions
about which node a partition will end up on as we expect them to fail
until they are updated.
* Change Ttl to TTL
Following go convention, acronyms should have a consistent case.
See
[Initialisms](https://github.com/golang/go/wiki/CodeReviewComments#initialisms)
This commit changes some public-facing methods, so any code importing
this package and using these methods will need to be updated.
* rewrite Ttl -> TTL
Co-authored-by: reesporte <reesedporter@gmail.com>
Fixed the issue where deselecting variables didn't work on quries like
groupby and extract.
The empty variables list returns All() when the variables are empty.
you can get a context deadline exceeded from clientv3 if there's heavy load and the
etcd server sends a grpc code DeadlineExceeded. this causes the etcd client to not
retry connecting and then you end up with nodes that can't recover.
Co-authored-by: tgruben <tgruben@gmail.com>
Co-authored-by: 54mir <48686912+54mir@users.noreply.github.com>
Co-authored-by: seebs <seebs@molecula.com>
Co-authored-by: tgruben <tgruben@gmail.com>
Co-authored-by: 54mir <48686912+54mir@users.noreply.github.com>
Co-authored-by: seebs <seebs@molecula.com>
The intent of these lines was "if there's no filter, return
immediately rather than doing operations".
But actually we didn't write that, so we were calling intersectionCallback
on empty filters, which didn't matter since it failed out quickly, but
it's still a waste of effort.
Except we shouldn't get to these anyway because ConsiderKey already
correctly rejected these cases. I think. But still.
In fact, we *do* want to skip ahead sometimes to the next thing, and
only call our callback for things that match our filter. I was thinking
that we needed to call the callback for all data regardless, because
what if you're writing to a mutex and adding new data.
But even if you're adding new data, it's still in the filter, because
it has to be, because we don't start out knowing there's no existing
data. So the mutex actually works fine.
So the rule for BitmapBitmapTrimmer is that your filter doesn't have
any meaning other than (1) it tells us which containers you need
to see, (2) we provide it to your callback function. Maybe you want
to subtract those. Maybe you want to add them. That's up to you to
decide.
Now that we have ApplyRewriter, it's a viable way to implement ImportMutex.
It can be slower on low-density writes, because it's checking more things
than it otherwise might -- the other filter form can skip ahead and only
check the containers it's modfying, in principle, while this one doesn't
know it can do that. (The decision as to how far to skip ahead has to
be made in the BitmapBitmapTrimmer, while it's the callback provided to
it that knows when it next has data to write.)
On the other hand, it's probably faster in some cases, and would be
more-faster if we could improve the cursor management a bit, and it's
skipping at least some seeking because it doesn't need to use
ImportPositions after reading the whole thing.
This uses the shiny new ApplyRewriter logic for ClearRecords,
mostly to verify that ApplyRewriter works at all.
This also implies separating the cache update code out from
importPositions so it can be used also by this.
We also use fragment.ClearRecords instead of the different clearFragment
code in executor. The clearFragment implementation did not update TopN
caches and the like. Standardize it on the clearRecords implementation
which does.
We don't need to manually copy each individual item and keep
checking for the second index being out of bounds for every
item, we know it can't change at this point, so we can just bump
it over. We want this operation because BitmapRewriter can
use it to simplify trimming in some cases.
The filter and rewrite logic are unlocking and relocking but I don't
think they should. I think those locks were added early on during
testing of the filter stuff, but I don't think they should be needed,
and I've been unable to find a case where they were. I think probably
I had something where a ConsiderData function was trying to run a Tx.
This in a parallel to ApplyFilter/BitmapFilter which allows writebacks
while it's running. It's a write operation, so it needs a write lock
on the Tx, and needs to create bitmaps if they don't already exist.
The semantics are a bit messy and need better documentation still.
We frequently want to grab the set of values from a []uint64
that correspond to a given key, and make a container from them, but
sometimes we only want to do one of these. This implementation
lets us do that the same way every time, and do in-place
container creation without extra allocs.
The test for bitmap-to-array succeeding doesn't work with roaringParanoia,
which *does* intentionally panic at that point. Possibly we should also
drop the corresponding logic that tries to prevent it from panicing,
since it won't work with the paranoia flag on anyway.
Follows the nextLink in http response to iterate through
paginated group membership response in order to obtain all
groups that the user is a member of.
Also, checks cache to make sure we don't add empty groups
to the cache.
Because the ToRowser interface was not implemented for DistinctTimestamp, there was
a error when using the GRPC endpoint to call Distinct(All(), field=ts). Implementing
the ToRowser interface for DistinctTimestamp solves that problem.
Related to SUP-210: WebUI, Python - Distinct() does not work for Timestamp field
If you're wondering how something that simple gets a commit
message this long, sit down, because you are in for a ride.
The Row, Rows, TopK, and GroupBy(Rows...) commands had three
different sets of semantics for from/to ranges. We unify these.
Sounds easy, right?
The original purpose of this was to address a bug in GroupBy
where, if you had multiple queries only one of which used time,
we could end up silently returning no results because we tried to
do a time query against a non-time field. This was easy to
fix; just move a boolean flag from outside a loop to inside
the loop so it resets to false on each pass.
In the process of trying to test that, I discovered that
specifying `from=...` without `to=...` in a Rows in a GroupBy
didn't work. Searching around, I discovered that we had three
different answers:
GroupBy, TopK: unspecified 'to=' is 0
Row: unspecified to is tomorrow
Rows: unspecified to is the max time quantum in the field
(A time value of 0 is apparently interpreted as January 1st,
0001.) Note that "GroupBy" is really referring to a Rows()
command in a GroupBy, it's just that this uses completely different
code (because it has to be computing rows potentially matching or
restricted to a filter, or provide the rows it generated so
they can be used to filter something else).
So we fixed that, and made a field method for finding the min/max
values (as done in a Rows command that *isn't* in a GroupBy),
and tried to use that with viewsByTimeRange. Then I tried to write
documentation for this, but the documentation was unclear, and
I tried to clear it up. Which caused me to discover that these
four different places ALSO differed in when or whether they'd
replace a broad query with "just the standard view".
So. Round two of the fix: We create a `field.viewsByTimeRange`,
which tries to fall back to a standard view when one exists
and the specified range covers everything, and treats zero
values as non-restrictive, but also picks a narrow range that
is actually related to the range of dates in the field. This
matters because viewsByTimeRange generates the entire set of
views it would need *even if those views don't exist*.
We drop one test that was testing Rows specifically to verify
that, if you omitted To, we acted as though you'd specified a date
two days in the future. That behavior is not now intended, so
we drop the test that tries to verify it.
Thing that might make this better: Figuring out a way to generate the
list of views more cheaply. Right now, we're redoing all the view
computation, including producing a sorted list of view names, for
every shard. This is excessive, but hard to fix.
In particular, there is no trivial way to generate a sorting such
that you can take slices of it and have them be the right slices,
because we want to skip smaller time quanta when an entire larger
parent quantum is included. e.g., if we're including all of
April 2022, we don't want to include any of the days for April of
2022, but if we're doing up through April 15th, we want to include
the first 15 days of April, but NOT include the whole-month quantum.
And so on. Fixing this cleanly is hard and would require a
significant design effort.
When you backup a cluster, we call /schema which marshals timestamp field options to
json, and restores the fields with those options. If the min and max are missing,
they are set to 0 on restore, which causes an issue on subsequent ingest.
Fixes [SUP-213](https://molecula.atlassian.net/browse/SUP-213) and
[FB-1332](https://molecula.atlassian.net/browse/FB-1332)
During backups on a multi-node cluster, TranslateData was unconditionally redirecting to the primary, regardless of the primary’s status. This is less than ideal. If the primary is down, the backup will fail.
As a consequence of this fix, we will also no longer needlessly redirect to ourselves on a single node cluster. This is a great optimization win!!!!
Fixes # FB-1324 SUP-209
Co-authored-by: tgruben <tgruben@gmail.com>
The ingest API tried to do a Qcx operation that needs a write Tx after
requesting a write Tx. This doesn't work. The ingest API is the only caller
of clearExistenceColumns, so it's easy to just make it work for a given
shard using a provided Tx. This isn't especially clean, but a lot of this
is due for an overhaul anyway because the Qcx/Tx stuff is sort of broken.
Also, add any test case at all for this, since we didn't have one, and
also fix the fact that the test case failed because the test harness
didn't allow empty result sets.
This commit addresses a bug in https://molecula.atlassian.net/browse/SUP-200 where
the Authorization header was not being set correctly when the token was passed via
"userinfo" in the context and not "token".
Now, we prefix the token with "Bearer " when the token comes from userinfo.
This commit also adds a unit test for this function, and simplifies logic. It also
fixes a test that didn't quite test the behavior we wanted.
this stems from https://molecula.atlassian.net/browse/SUP-194 where the string
"standard" was being passed to viewTimePart, which output "standard" as the result.
this is not a valid time string and was causing confusing errors. now it simply
doesn't do that
this commit also adds regression testing framework and a regression test for fb-1287
queries can become arbitrarily long when variables are used.
This is especially the case when a variable is defined as
'select distinct field from table'
and a user wants to use all the values in a Row call
(which is effectively disabling any condition on the field).
This change allows users to select no values for a variable
associated with a Row call to disable the condition. If that
variable is the only condition (query expands to nothing)
then it interprets it as an All call.
Prior to this, we locked the index-wide i.mu around writes to i.fields.
But now the entire open process is holding that lock, so we couldn't
lock it here (that'd deadlock) and didn't think we needed to (because
it was held). But in fact that means that multiple fields being opened
at once can concurrently write to the map. Conveniently, we *already*
pass a shared mutex into openField() to prevent exactly this sort of
problem; we just need to actually use it when doing the write.
This is a lot more complex than it sounds like it will be.
We shut down the cache flush when a holder is closed, but if you're
deleting an index, we don't check for that, and can have a cache flush
still creating cache files in an index which could conceivably result
in os.RemoteAll() failing. This shouldn't happen often, but it's happened
at least once.
To address this, first, we make sure that every tier of this operation
bails as quickly as it can after the thing it's working on closes. Second,
we retry RemoveAll.
Unfortunately, some things get reopened, so we have to handle that,
have mutexes covering the access to the channel, and so on. Also, some
things were getting double-closed, which was previously harmless but
could now fail. So, first, catch all the existing double-closes and
remove them, second, make the double-close fail with an error. Note
that virtually none of the tests check for errors on close.
This passes tests and should be unable to hit the original problem.
Unfortunately, it's unreasonably hard to check that, because it
requires an incredible coincidence of timing on the delete aligning
with a cache flush.
MacOS's firewall complains about a previously unknown app trying to
listen for network connections whenever we run go test. That's because
we *are* listening for network connections on arbitrary interfaces, not
just on localhost as we probably intended. Fix that.
If non-primary host fails to process a request, retry on primary node.
conditions when we should not do this:
- no error
- we've aleady tried the primary
- we're making a status request to get the primary node...this
could lead to lock contention if we allow it to happen as we are
making an http request within an on going http request to discover
the primary node.
This also deletes the RemoveHost method and the associated test b/c
it is not used anywhere anymore and updates the returned error type.
We have code to correctly fill in cell.BitN when a leaf cell already
exists but isn't of the correct sort, but not to handle the case where
it already exists and *is* a BitmapPtr, but doesn't necessarily have
the right BitN value.
This test tries to verify that we can create multiple fields on a cluster
without deadlocking or getting errors *other than* ErrFieldExists or
wrappers of it. The "or wrappers of it" implies a change to ConflictError's
semantics, but honestly I think it should have had those semantics all along.
Two CreateField messages reaching different nodes in a cluster at the same
time could cause a deadlock because each CreateField runs with a write lock
held, then issues requests to other nodes which, at a minimum, need
a read lock and which may require a write lock. Reorder things a bit to
make the broadcast to other nodes happen outside the lock. We may also
need to do something to have nodes handle the case where something's been
created in etcd but they haven't gotten the message about it yet.
Creates a timestamp field in the TestSQLQuery dataset.
Modifies a helper function to allow datasets with
timestamp to be properly converted to table responses.
Adds test cases for:
- conditional where clauses
- where clause with group by
- timestamp within where clause
- select distinct with where clause
This was in response to some feedback we got about the new release
format. Executables were no longer had executable permission due to
going through S3 (hence the tarballs), and we wanted a more consistent
directory structure in the final release which included the versions
of various components.
this is related to work for [fb-1127](https://molecula.atlassian.net/browse/FB-1127)
cardinality reporting has caused no shortage of issues such that we recommend
disabling them almost everywhere.
this commit removes the cardinality calculation for right now, as well as the option
to enable/disable schema details.
If a field doesn't exist, looking up that field produces a nil,
and querying the name of a nil field fails. Don't do that. Instead,
just use the name you're looking it up by.
We could in theory return an error here, but we already handle
nonexistent fields elsewhere and checking this when we already have
checks for it seems unnecessary, I think?
Also, we add a test for this. The test is over in server/grpc_test.go
because we have infrastructure there for testing the SQL server
functionality, and you can't actually write reasonable self-contained
tests for the SQL stuff because it has no way to create a working
server.
We create a lot of these during a large GroupBy query or anything else
that creates a ton of filters. Use a pool so we can reuse them, since
most of their data doesn't need to be zeroed out, and typical use
patterns have a lot of sequential creation of these short-lived things
within a goroutine.
We don't really need to fully extract every row, we just need counts.
This naive approach uses logic similar to BitmapBitmapFilter, but tweaks
it so that we can intercept the existence and sign bit rows, work with
an optional filter, and yield a sum. We accumulate the statistics
internally, rather than using a callback, because I tried to make it
work with a callback and it was a complete mess.
Note the fancy check for container reuse in the BSI Count filter.
This is because intersection(full container, X) is just the original
X, *not* a copy, but in this case we need a copy because RBF
ApplyFilter will in fact reuse a single container's storage for
each consecutive container.
We used to manually do this because we had a number of cases where
BitN wasn't being updated, but so far as we know we've fixed them
and we have run a fair amount of stuff with sanity checks on and
not hit anything, so eliminating the constant recounting on bitwise
containers seems like a win.
We have some tests that cover stuff like the DumpDot functionality,
but we don't need them to actually write to stdout during ordinary
testing. Dump to buffers which we politely ignore. Yes, we could have
used a dummy writer, but this way it's super easy to display the
contents if we find ourselves suddenly caring.
When closing, we need to wait for existing Tx to exit before truncating
files and unmapping things. This shouldn't matter, because we don't actually
close the DB until all transactions are done, normally... except for the
background usage-gathering task. But really, it's probably just better to
be conservative.
The actual logic is fancier than it looks. We can't hold db.mu.Lock during
this, or the existing Tx can't exit. So we first grab the lock, set the closed
flag, set up a waiter for all current Tx to exit, and then release the lock.
Now we wait on the current Tx exiting. Once that's done, we grab the locks.
Anything coming in that tries to start a Tx will fail out fairly quickly
because the opened flag is now false, so even if other things get those
locks before we do, they won't keep them or create new Tx.
This makes one test deadlock because it opens a Tx and never closes it,
so we change that test to close its Tx.
Check if queries that have a 'like' argument are applied to keyed
fields. If not, log that the user is trying to use 'like' on an
unsupported field type (as opposed to reporting that there
are no results.)
This should help prevent a data race. SetBit can, in some cases, cause an
asynchronous task to run which tries to update the stats counter.
But if that task runs while we are modifying the stats counter itself, we have a
data race.
Add a lock to the Server WaitGroup so that if the Server WaitGroup is already
waiting, we won't concurrently add to it and cause a data race.
Also, when adding to the Server WaitGroup, check that the server is not closing
already, since that means we really shouldn't be doing more work.
When deletion is started, _exists field is updated with row+1.
After deletion is completed, we delete _exists=row+1.
If _exists>=1, then deletion was not completed.
Updated go version in docker to match other requirements.
Removed duplicate error check for grpc.
We recover from some specific panics deeper in the PEG parser, but when
we added the invalid timestamp, we didn't add it to the list we catch
and handle gracefully. Add test case for this, and test case for
successful parsing. Also add the word "valid" to the error message so
people don't get as confused by it.
There's an obvious bug, plus another bug that I hit trying to reproduce
the first bug, plus another... it's a long story.
Basically: If you get nothing back from executeDistinctShardBSI on a
Timestamp field, the request for a large enough pool of strings to hold
timestamp conversions of the nothing segfaults because r.Columns() on
a nil row segfaults.
To try to test this better, I added a filter to the executor test that
we use for this case, which got me a different result complaining about
a DistinctTimestamp result not being a SignedRow.
So, there's a couple of issues. One is that, in the case where a filter
is present, if the filter comes up with nothing, we can bail early
and return a result of the SignedRow type, which then breaks the reduce
part of our map/reduce when we try to reduce DistinctTimestamp values
into a SignedRow. To fix this, we make sure that we return the expected
type even in the case where we're bailing early.
A simpler way to see the actual original bug is, rather than having
a filter, just have a shard that has a value in *some other field*
but not in the timestamp field. So we add that to the test, too.
But also, really, since this is a problem that's happened more than
once, I propose that we also just make nil rows allow you to request
their columns and get back nil, so things like this don't bite us as
much. This wouldn't be a sufficient fix for the filter case, and I
still have the short-circuit for the nil row case explicitly in this
particular case because relying on the nil behavior bugs me, but I
think it's safer to allow .Columns on nil rows.
empty string doesn't work because gitlab doesn't set the variable at
all. How do I know that "null" is correct? Because Fletcher told
me... apparently it's a ruby-ism
This commit changes `RankCache.BulkAdd()` so that entries are
limited to an upper bound of 2x `maxEntries`. When this bound
is exceeded then the cache is automatically recalculated.
-This code should cover the retry logic.
-The ingest is set to 100,000 records, because it's difficult to cause
leader change with lower number of records to ingest.
-Additional node: By applying stress on other two nodes while ingesting
simultaniously causes "context erorr" logic which can fail many tests.
This adopts the task pool functionality to let us spawn new worker
threads when worker threads are blocked. The underlying reason for
this is the same as the reason for the previous worker-pool-growing
strategy; while our design persistently has at least one thing which
can proceed, it can be the case that there are N things blocked,
where N is the size of our worker pool. Blocked workers shouldn't
count against our desired number of workers.
Originally, the intent was to thread this into RBF, and provide
backpressure from RBF on the pool when blocking on writes. Unfortunately,
that's not good enough, because while a write is blocked, the Qcx
calling it is *also* holding the Qcx's mutex, which means that any other
NewTx on that Qcx will *also* block. So we need to block for the
entire time of the NewTx.
Removing the existing worker spawning code resulted in a subtle
and maybe-harmless change; prior to this, each invocation of `mapperLocal`
would hold a lock, which meant that all the tasks for a given local mapper
would be put in the queue *sequentially*, ensuring that they'd all be
picked up by workers before things from later workers.
With the new pushback, that's not, strictly, necessary. Also, if you
disable it, you can end up with 300,000 goroutines at once, most of them
blocked.
A smallish run does, in fact, eventually complete anyway -- it will
indeed keep making workers until everything gets one. However, while
it's *correct*, it's also noticably *slower*. The same test workload
goes from around 33 seconds to a bit over 40 seconds when that lock
isn't present. (But that's with an extremely small WAL write cap
introduced to make the previous deadlock possible.)
With large numbers of shards, the practical impact is that you can
have quite a lot of things in process, with hundreds of goroutines
each, all blocked waiting for one writer. If we force them to all be
processed at the same time, all the reads that are connected to
each other are much more likely to get all processed at once, before
something new comes along.
In short, that lock isn't strictly necessary but it seems to help
noticably with performance and reduce simultaneous goroutines
significantly.
This implements a task pool which can handle backpressure; the
idea is, you have a target number of workers, but when a worker
blocks, you can tell it that it's blocking, and it can spawn
another worker in the mean time. This reduces the bounding provided
by the worker pool, and can significantly overshoot the intended size
of the pool in some cases, but it provides quick scaling up when
part of a workload gets blocked.
There's also a simulator attached to it. The simulator's job is
to act similarly to the executor's worker pool working on RBF
databases, including the weird semantics of writes and reads;
specifically, that reads aren't blocked by writes, but a write
can't terminate until every read that started before it has exited.
(This is an oversimplification; actually, writes can complete,
but they still hold the write lock until any WAL merge completes,
and the WAL merge can't complete until old reads are done.)
The simulator is significantly more complicated than the pool.
- For server side, used an instrumented binary with a test that wraps around the main entrypoint for featurebase
- Every time, the binary is called, a new coverage file is generated.
- For the client side, used the standard -coverprofile flag for go test to generate code coverage
- For backup test that's expected to fail, needed to call Run call in backup.go directly. The code coverage is not written to disk for an instrumented binary if there is an error.
We have pipelines that get to the gauntlet stage then get failed because
the ASG scales-in before the gauntlet stage finishes. (4/6 of the last
gauntlet failures were from this failure.)
There are a few ways to fix this, but my proposal is to turn on scale-in
protection to stop scaling in the instance running the gauntlet job
(scale in other instances instead), then turn off the scale-in
protection after the gauntlet test is run.
cluster.Start creates ephemeral ports for all the etcd stuff, whereas
node.Start uses the default config. I don't know why this test was
using the node.Start, but it passes without it.
messing around trying to get Rows call to recognize
$ syntax. got Rows to not barf, but it is interpreting $ syntax
as string values for the _field parameter as opposed to a Variable
- rip out gobby stuff
- add tokenCache, groupsCache
- refresh the token if needed
- set cookies after authenticate
- remove signature validation, the IDP does that for us
- added way more unit tests
- update older tests to use new API
- add fake idp to authcluster tests
this is necessary as in some cases we want a low timeout (when we
expect a quick response, e.g. with backup), but in others we may want
a very long timeout (long running query).
Now we have more granular control over timeouts so we can get things
to fail more predictably in tests.
this should be a lot more reliable than trying to construct it based
on the project name as the exact construction can differ between
docker-compose versions.
There was also an issue with the backups succeeding when they should
fail in the test. There's an arcane maze of HTTP timeouts to navigate
here, but basically there are situations where the client will just
wait forever rather than erroring if the server is paused at the
right(wrong) time. I'm not convinced we've solved every possible case
of this, so we still may see the backup succeed even when it's
supposed to fail. The ultimate hammer is to add Client.Timeout, but
that's a very blunt instrument and I'm afraid it could cause a timeout
when really we just have a lot of data to download or something.
There may be a better way to say "only time out if you literally
haven't heard a peep from the server in this long", but I haven't been
able to figure it out yet.
I also fixed how the authclustertests are run as they weren't using
the PROJECT parameter correctly. Now they can run concurrently with
clustertests, and with other copies of authclustertests without having
conflicts.
- Add auth-token for featurebase import, backup and restore
- Add auth-token to http request
- Create a cluster tests with auth enabled
- Add test for import with auth enabled
- if we're a non-primary node, redirect to the primary
- if non-primary nodes can create transactions now, then the client should not receive an ErrNotPrimaryNode
- streamline metrics logic
it was somewhat difficult to avoid ripping this out without also
touching some of the stuff that supports roaring backend. That's going
soon too, so no worries :)
for the following auth related packages:
* authn
* http
* server
fix minor bugs, do some cleaning up, etc in `authn/authenticate.go` and `http/handler.go`
The default etcd config means that if two of this test run around the
same time, we end up with one of them failing because it can't bind.
Elsewhere, we resolve this by binding to ephemeral ports and fixing
up the config to use them, so we duplicate that here.
This includes duplicating the existing listenerWithURL from test/,
because that package has to import us, so we can't import it, and
I don't really want to make a separate package for one trivial
function.
There's no correct timeout value here, really, but the intent
of this is that we first want to be sure that a second tx doesn't
successfully start before the first exits, and then that the second
*does* successfully start *after* the first exits.
Unfortunately, there's no guarantees on timely processing, and in
reality, CI can break us by waiting more than 10ms before we get
enough CPU time to do something. More generally, there's no way to
make a test like this work correctly -- no matter how long you wait
for the second Tx to start before closing the first one, it's always
possible that it *would* have started just a millisecond later even
without you closing the first one. And similarly, no matter how long
you give it to start when it's *supposed* to, it could always take
longer.
We could in principle just set this to wait for the second Tx to start
and rely on the test timeout killing us if it doesn't, but then we
don't get a useful message.
Let's optimistically hope that 10 seconds is long enough for a trivial
rollback to happen, since that doesn't need to imply writes. And I
think 50ms is a better bet for the first test, although that does
make this test close to 5x slower on non-CI hardware.
This should always be etcdserver.ErrLeaderChanged, but actually
apparently it's not always:
non-retryable error: etcdserver: leader changed
The "non-retryable" comes from our code. The "leader changed"
message appears to come from etcdserver, but there appear to be
circumstances where it has a suffix, or it could get wrapped,
so we check for the string being contained in an error. This is
not pretty.
gitlab CI runs as much as 5x slower sometimes during business hours,
resulting in tests failing due to 10-11 minute timeouts that would
succeed in under 2-3 minutes outside of business hours. to allow us
to do anything at all, let's just set that to half an hour, and 90
minutes for `go test -race`.
Concern: It's possible there's a timeout that's a gitlab CI configuration
thing involved too, because we see some go test timeout panics, but we
also see some weird messages about SIGQUIT at 11 minutes, which isn't
the go test timeout, so we may need to address that too.
Note that we're changing the Makefile, and also the config for the
gitlab CI passes, which don't use the Makefile. The Makefile changes
are just to be careful and avoid retriggering this later. We may
want to revert these if we get the other issues fixed.
This affects TestTx_Remove, TestTx_DeallocateToFreeList, and
TestTx_RecreateBitmap, all of which were adding hundreds of thousands
of individual bits, or more, and all of which work just as well and
produce the same behavior using largeish containers.
This reduces race-detector-test runtime from about 20 minutes to
a couple.
The MultiTx test runs for a fairly long time but doesn't add much
value running that much longer, and there's no reason it should take
more than half the time we spend on this entire directory.
The Cursor datatype is quite large, and allocating them constantly for
ops is extremely expensive. To avoid this, we create a single stable cursor
that lives in the DB, and can be used for freelist modifications. Since the
freelist is only ever modified once at a time, this should be safe. We also
don't fully zero it between operations, we just reset the relevant parts.
Several changes. One is, we don't provide a `New` for pagePool, which
allows allocPage to check whether a page was returned, and thus, zero
pages which were found in the pool, or make new pages, but never zero
pages it just created with make. We then also make many more things
which were making pages use the pool.
Reuse the same page allocation for multiple header pages dumped into
the WAL; the bitmap header pages aren't stashed in our page map,
they're only written to the disk, so we don't need to make a new page
each time, we can just make one new page for the whole batch.
Internally in the pool, we pool pointers to [PageSize]byte, rather
than slices. sync.Pool needs pointer-like things. To store a pointer
to a slice, you have to heap-allocate the slice, also. So, instead
of heap-allocating copies of these slices, we just use pointers to
the raw data.
the shardwidth22 tests were broken client side, but we didn't realize
this because we weren't running the client side tests since moving the
client code into the main FB repo until recently (woops), and more
recently, we'd stopped running the shardwidth22 tests in the move to
Gitlab, so when we re-enabled them we finally noticed that they were
broken in the client.
All this change does is takes the shardWidth value from the core
featurebase package instead of using a hardcoded value in the client package.
addresses ticket FB-1109:
when auth is turned on, we log:
- source ip (if available)
- user-agent
- user id
- user name
- query string
- request endpoint
also adds some minor tweaks and comments to chkAuthZ flow
I was going to write a docker-compose thing for this to run postgres
alongside the Go tests, but then saw that Gilab has this handy-dandy
notion of a service, so used that.
we had a CI job fail in an interesting way, but can't tell if the
etcd retrying stuff is working, so adding in this wrapping so we can
better differentiate the errors if we see it again.
Job is here: https://gitlab.com/molecula/featurebase/-/jobs/1977060827
Failure is:
```
=== RUN TestClusterStuff
cluster_test.go:36: creating index: against http://pilosa2:10101/index/testidx 404 Not Found: 'creating index: sending CreateIndex message: executing request: against http://pilosa3:10101/internal/cluster/message 500 Internal Server Error: 'processing message: getting index: testidx: etcdserver: request timed out
''
--- FAIL: TestClusterStuff (8.85s)
```
* fb-998 - authn/z enabled in handlers (kitchen-sink ticket)
- authorization is enabled through the use of a bearer token (using header "Authorization")
- authorization may occur through the use of an "Authorization" header or "molecula-chip" cookie
- ui is updated for changes to handler
* fb-1131 - protect grpc endpoints
- GRPC endpoints now check authorization if auth is enabled
* fb-1129 - inter-node communication
- the following endpoints use the secretKey for authentication:
- /internal/cluster/message: POST
- /internal/translate/data: GET, POST
* added test to api_test.go (TestAuth_MultiNode) testing various auth/permissions stuff on a multi-node cluster
not included:
- fb-1130 - filter response of endpoints
- fb-1109 - improved audit logging
@jaffee [are you not entertained](https://www.youtube.com/watch?v=mutgotxrcqg)
Co-authored-by: souhailanoor <90720110+souhailanoor@users.noreply.github.com>
Co-authored-by: tgruben <tgruben@gmail.com>
Co-authored-by: 54mir <48686912+54mir@users.noreply.github.com>
Co-authored-by: kcrodgers24 <49999391+kcrodgers24@users.noreply.github.com>
The central reason this exists:
**sync.RWMutex can block read locks even when no write lock is yet held.**
If a write lock is *requested*, this can block future read locks. In
particular, this means that recursive read locks are unsafe. But there's
additional problems.
The specific case that bit us involves not two, but *three* things
running at once.
Thing #1: executor doing AvailableShards. This RLocks the index, and
then each field, and then each view. To complete, it must be able to
obtain a read lock on each view in turn.
Thing #2: DeleteField. This Locks the index. Even if it is stuck
waiting for the lock (which it will be until AvailableShards completes),
it can prevent *additional* RLocks of the index.
Thing #3: CreateFragment. This Locks a view, then RLocks the index in
order to look up a field.
CreateFragment can't proceed until DeleteField completes. DeleteField
can't proceed until AvailableShards completes. And AvailableShards
can't proceed until CreateFragment completes.
Solution: Cache the *Field in the view, so we don't need a read lock
on the field or index to complete a CreateFragment.
This tries to be more correct/careful about retries (checking against
the actual exported errors from etcdserver, not just the string
representations), and also supports retrying on timeouts, not just
on client changes. It can also retry more than once, mostly in case
we hit one of each of those.
For timeout errors, we mostly use the fact that it's a timeout to
give us a reasonable backoff, but then delay a fraction of a second
longer just to give it a moment to recover if the ErrTimeout is
masking something else that took longer.
This commit changes the max WAL size calculation to double the
number of bitmap pages in the WAL as they require an extra header
page. Previously, this was causing the WAL to be overrun and
references to those pages were outside the mmap range and caused a
panic.
also use a single cluster with each test creating a different index
rather than each test creating a whole new cluster.
runtime went from 38s to 30s in my informal tests
discovered that client tests weren't running due to integration build
tag. Fixed the file I needed to get through SonarCloud and documented
rest of what needs to be done in FB-1152 https://molecula.atlassian.net/browse/FB-1152
looks like test-report.out and coverage.out aren't about the same
tests. I'm unclear on how sonar uses tests.reportPaths vs
coverage.reportPaths, but figured I'd try at least generating them
from the same run to see if that helped.
fixes pathological case where imports with randomly ordered IDs which
spanned multiple shards and included ints or mutex fields could be
incredibly slow due to making 1000s of requests.
based on staticcheck results:
server/server.go:627:58: file mode '600' evaluates to 01130; did you mean '0600'? (SA9002)
server/server.go:632:65: file mode '600' evaluates to 01130; did you mean '0600'? (SA9002)
This adds the Terraform needed to create a gauntlet testing framework for a cluster that is a mirror of Samsung's. It is meant to be run once a day in CI via the GitLab scheduler.
This commit effectively removes the API-level validation that was
blocking certain API methods when the cluster was in a particular state
(namely DOWN and DEGRADED). The thinking is that we shouldn't be
blocking these requests at the API level, but rather should let them
pass through and allow the fact that a node is ACTUALLY down dictate the
behavior.
With this change, two tests were modified. They were previously
expecting the error message from the API validation on DOWN, but now
they check for a "shard unavailable" error, which is what gets returned
for a particular query when the cluster is in an unhealthy state.
A few things were going wrong here.
First, we take a "RetryPeriod" option on backup and restore which is
meant to be roughly the total amount of time we spend retrying any
given request before failing. However we were incorrectly passing that
as the RetryMaxWait which is the maximum amount of time to sleep
between any two attempts. We now do some fuzzy math to figure out
approximately how many attempts we should make given a minimum sleep
of 100ms and the fact that we double the sleep time every attempt.
Second, during the backup test, if a host was totally stopped when we
started the request, it would fail immediately and then retry, but if
the host was stopped during the request (after DNS had resolved), then
the request would wait for the DialTimeout which we default to 30s, so
turning off the cluster for 5 seconds and turning it back on resulted
in the backup completing rather than failing. Because of this, we
change the commandClient to have a default dial timeout of 1 second.
I was tempted to change the global default to 1s which I think would
be fine, but didn't want to break anything too badly.
instead of awkwardly reading an entire file into a buffer, we use
retryablehttp's reader func to open the file fresh if we need to
retry, so a small fixed-size buffer can be used internally for copying
the contents onto the network.
this is really not ideal, and there are libraries for this kind of
thing, but I'd have to figure out how to make the libraries work with
everywhere we're already creating stdlib http clients.
There's a number of deeper issues here (the fragment is conjuring
up a Tx, for instance) but this helps.
Also use field.view() to get the view rather than accessing viewMap
directly without a lock. Also change field.cacheBitDepth to ratchet
upwards -- if we have multiple shards and some shards have lower
depths than others, we should use the highest as the cached value,
not the most recent.
This commit fixes a bug in RBF where deleting all the elements in
a bitmap that has a depth greater than 2 will cause the root bitmap
to be a branch page with a cell count of zero. This breaks an
assertion in `readBranchCell()` which causes a panic post-commit.
A new assertion has been added to prevent a branch page from being
written with a zero count in the future.
if we have more than twice our starting worker pool, and have had no
tasks when checking the queue for multiple rounds, send a job telling
the system to retire a worker. eventually we'll get down to about 2x
the starting pool size if we stay idle.
We don't need a condition variable for a thing with a single waiter
which waits only once, and a data structure which only one side ever
modifies. That's a closable channel.
Two issues: First, there was a race condition because we were never
using the mutex for anything but the condvar broadcast, second, there
was no reason for the afterCurrentTx to need to maintain the list since
we already know where in the list we are when we are waking it up.
afterCurrentTx still wants to run with the db lock held, because
the degenerate case (no outstanding Tx) means that it will be running
with it held already. That's for another commit.
We need to update db.PageMap after we write the db, but before
we truncate the WAL, so new transactions don't pick up the old
PageMap and then get a truncated WAL.
Also, checkpoint should not abort if there's txs -- that's okay now.
When a qcx is a write, every Tx under it closes immediately, thus
invalidating all returned data. Thus, if you do a Not() inside a Store(),
you're doing a difference on an existence row and some other row
call... and both of those rows were run, individually, as separate
transactions that got invalidated the moment they were fetched. Oops.
This gets us to being able to run reads during a checkpoint, but
now we have to wait for new reads to end before we can release
the write lock, etc.
This is actually slightly slower, but if we could get ONE more step,
we could allow new writes during that phase, to a different WAL,
if we had a different WAL to write to.
PageMap uses "WALID", which is a WAL page ID relative to the "base" ID of the
WAL, rather than the wal page count you'd get just reading the file. So everything
it reports has a fixed offset at any given time. I think this may be left
over from a point where there were partial checkpoints. Anyway, the net
outcome is that each new transaction was getting different page IDs, but
the actual WAL pages did not always reflect that. Each checkpoint increases
the offset. This might imply that we can start having problems after
4 billion pages written even if most of them were redundant?
Anyway, with that fixed, this seems to work. I think.
We change nothing substantive here, except that there's a window
between when a write transaction updates the root pages and when
it removes itself from the db tx list and possibly causes a checkpoint
where it's not holding the db lock.
The issue here is that we want to be able to *keep* the lock but
still return, so no one else can start transactions, but the specific
Rollback or Commit that removed the last outstanding transaction
doesn't block forever. This will, later, allow us to exercise
finer-grained control over when we allow transactions. This is
a separate commit so we can run the test suite against it, and
verify that this part in particular didn't break anything.
Discovered test was running slightly strange and spending an unreasonable
amount of time on rand.Intn(), possibly because we weren't caching the
value used as the loop condition. Tweaked that, also made the pool a
bit different. Now it takes ~50 seconds for benchtime 100x, and produces
a profile with a TON of time spent waiting on sleeps (expected) and
the condition variable for waiting on checkpoints (the thing we want
to measure, really).
Note also the commented-out debug printf in checkpoint, there as a
reference. This is interesting because it turns out that MOST of checkpoint
writes is not actually writing new pages in most cases.
The actual "pages in WAL : pages in map" ratio is typically around 30:1
apparently. This would likely be different in cases where we were
updating existing data, though.
This is scratch space to prep for an actual work. The final
results will likely be different.
also found a weird issue with schema marshalling
if you create a field thru the api w/o specifying a field type, you
get slightly different behavior than going thru the HTTP handler which
is... not ideal. I changed the marshaler to accept an empty field type.
i used this script, a little clunky but it got the job done
```bash
for file in `find . -type f -print | grep '\.go'`; do
sed '1,/^\/\/ limitations under the License.$/d' $file > $file.tmp;
result=`cat $file.tmp`
if [[ result != "" ]]; then
gofmt $file.tmp &> /dev/null;
if [[ $? == 0 ]]; then
mv $file.tmp $file && gofmt -w $file;
else
rm $file.tmp;
fi
else
rm $file.tmp;
fi
done
```
There isn't really a field called _keys but some old backups
will think they have translate data for this. Ignore it politely.
Also in general produce a diagnostic rather than a panic for
translate data restores to nonexistent indexes or fields.
We were trying to write an error to a ResponseWriter After attempting to
write to it, and this produces messages about superfluous WriteHeaders,
which is correct. This patch changes things so that we report messages
more clearly and verbosely if we hit them before writing, and if we try
to write and fail, we log the message because that's all we can do.
This does change semantics slightly, in that now we're marshalling
separately from trying to write the marshalled data. I think this is
probably a reasonable call because it lets us get diagnostics about a
hypothetical encoding problem, but in practice I don't think there
should be any encoding problems. So my guess is the actual error will
occur in that last line, and be logged to the server console instead
of failing to write over HTTP.
Also note that this changes some of the messages to include the
underlying error they're complaining about.
We also merge the create/find and index/field cases because only a
couple of lines of code changed between four largeish functions,
and we test some of the failure cases.
We don't have test coverage on the "field isn't provided" type things
because the mux won't actually route things there without them, so
far as I know.
SanityCheckMapping is specific to roaring bitmaps stored in-memory, if
we have an RBF backend, we shouldn't even try it, it'll just panic.
This implies that, in whatever circumstance we were hitting this, we
were getting an error back from the backend. We still need to address
that error, but to do that we need to know what it was, which we don't
if we panic.
we're no longer Apache 2.0 licensed, or open source, so LICENSE and
CONTRIBUTING.MD are gone. We track the changelog elsewhere, so that
can go, and I don't think anyone has looked at the NOTES file in 3
years. I modified the NOTICE not to refer to the Apache license any
more.
First, it is possible for us to end up allocating *or freeing* pages during
a modification of the free list, in a way such that the change to the free list
means that when we finish the modification which caused the allocate or free,
we've overwritten the inner change.
Second, when deallocating trees, we don't actually deallocate the branch nodes
themselves.
The former causes potentially severe data corruption. The latter causes us
to gradually leak pages in a way that we don't notice because we only run those
tests during the RBF tests.
The fix for this is surprisingly intricate, because of the counterintuitive
fact that *allocating* a page means *removing* things from the free list
(and thus potentially deallocating free list pages), while *freeing* a page
means *adding* things to the free list (and thus potentially needing to
allocate pages for the free list).
While modifying the free list, any allocations we need always just come from
the end of the file; we don't try to reuse free pages. If a page becomes
*deallocated* by a free list modification, we don't annotate it in the free
list at the instant that it happens; we stash that information until the
current modification of the free list happens, then iterate through any
such pages.
I am pretty sure there's virtually never more than one, and I don't actually
know that I can create a case wherein we'd end up with the nested case
firing, wherein removing a page from the free list causes us to remove another
page, but I think if the free list got large and cluttered and needed
rebalancing or something it could maybe happen.
The "just open the holder" subcommand doesn't work the way it used
to, because now that we rely on etcd to open a holder, trying to open
a holder without things set up just coredumps.
Step 1: Fix that.
Step 2: Also add a test that covers it so we don't get bitrotted again.
Step 3: Remove an unrelated stale comment that doesn't deserve its
own commit log, having to do with an option that no longer exists
which is no longer being set right under the comment saying we set it.
I'm not actually sold on this, but I'm not entirely unsold on it. It seems like
it does reduce the amount of duplication a lot, but also it's sort of a mess.
In the process, noticed that it makes more sense to grab the whole cluster
rather than just the nodes for an arbitrary shard for the shard==^0 case,
because then if we have an API (but no Qcx), we can be reasonably confident
that we'll be able to pick the local node for loopback even if we aren't
using the API directly.
Have thought about whether we should create our own Qcx in cases like that
but I really don't like the idea of automatically creating a Qcx.
Underlying goal: Don't use the http client to send messages back to the
local host. Also, when sending data to other nodes, don't collate it
from an ImportRequest into a completely different format, then immediately
collate that back into an ImportRequest. This does require changing
the logic over in ctl/import to make it create an ImportRequest.
Also, add additional testing to make sure we're actually trying anything
at all with several combinations (such as submitting import requests
which don't match the configuration of index or field), and improve
test coverage for that.
This introduces the ability to tell an http/client InternalClient about
a specific API that it should use for local queries where applicable.
That's not implemented outside of the import stuff, but should probably
be applied eventually to other things that are trying to talk to many
nodes one of which may be the local node. That behavior is contingent
on passing in a Qcx, because it is implicitly tied to an existing
execution context, and it can't assume that it can create a new one,
because that could deadlock.
Reusing the field operation sorting for other things caused me
to hit a bug, also made me curious about a performance issue and
whether it was possible to improve it. Answer: Not easy to improve,
anyway.
try to fix yml syntax
same
same
same
same2
same3
same4
same5
try with shell runner instead of dind
remove lattice from dockerfile
change path to bin
runs after linux arm64 build
change dockerfile path
same
same
add dir
better test coverage
The old revision emits a warning on MacOS X that looks concerning, and
even though it's actually mostly-harmless, it is an annoyance.
Also run `go mod tidy` which affected go.sum.
This is targeted at reducing startup times, especially on OSX where
the fsync calls seem to be taking an egregiously long time. I got one
index to go from ~1min to open to ~1sec. This looks safe to me, but
will get opinions from RBF experts.
These commits are hard to disentagle, and doing them separately means
re-modifying the same chunks of code several times before removing it,
and similar things.
Basically:
(1) Drop the bolt backend storage.
(2) Drop the blue-green wrapper that compares two backends.
(3) Drop unused or barely-used Tx API components from all the
remaining backends.
(4) Minor related cleanup to simplify things related to these.
The boltdb backend existed only to verify RBF. The blue-green wrapper
was mostly used to verify RBF, but in practice we had to do a lot
of working around that, and it introduced a lot of special cases.
Types removed:
IteratorFinder: Used only to implement the roaring iterator
on top of boltdb, and to complicate the way it worked in roaring.
Reverted the complications. Also unexport NewSliceContainers
which is used only for that outside of roaring's internals.
PortMapper from cluster_internal_test.go: Used only for a test
we removed early this year. Never used for anything else.
RawRoaringData: Totally unused.
TxStore: Totally unused.
Functions removed from Tx API, and sometimes corresponding
members were removed from structs:
* Dump: debugging code, I don't think I found any actually reachable
paths to it.
* Group: only used for debugging TxGroup stuff
* IncrementOpN: only used by fragment, fragment can increment its
own opN.
* Options: unused?
* Pointer: debugging only
* Readonly: used only to decide how to handle Tx in a TxGrp,
but we never add a non-readonly Tx to a TxGrp. Removed also all
the corresponding write-aware stuff.
* RoaringBitmapReader: Used exactly once, can just be a bm.WriteTo.
* Sn (and OpenSnList): Unused
* UnionInPlace: unused and conceptually-invalid; it didn't write
to storage and shouldn't have, and was just "create a bitmap
then call union-in-place", which we can do directly.
* UseRowCache: just checked storage.UseRowCache.
Other things removed:
The SetRequiredForAtomicWriteTx and ClearRequiredForAtomicWriteTx
functions go away, since nothing now seems to be using them? Same
for holder_internal_test's `testHasBit` and `testMustNotHaveBit`,
which were unused.
The DBPerShard "DeleteDBPath" and "HasData" functions and related
parts were mostly unused; took out the parts that were never
actually being reached.
Changed the API of one function to simplify special cases and
remove things:
* ImportRoaringBits had a special "data" argument which gave it
subtly different semantics for RBF and roaring (for roaring, it
could produce a roaring bitmap *with ops log*), didn't seem to
be adding much. Removed corresponding "readStorageFromArchive"
which is not otherwise used.
Also took out various debugging/dumping functions that were unused
and may have bitrotted.
Dropped a test from txfactory_internal_test, and the "pjobs"
code, because those two were the only things that needed Barrier
and thus idem, which lets us drop two more dependencies. We already
have errgroup for grouping things which want to terminate as
soon as one of them errors, approximately. To do better we'd have
to have context-threading, really.
Unbroke the WriteFragment test for non-roaring tests and made it
not roaring-only.
This also requires doing something to keep the TxGroup in each Qcx
from holding its Tx references after the Qcx closes, because otherwise
the list of Qcxs that we keep to verify that they all got closed ends
up keeping every shared/read-only Tx open forever, resulting in many
gigabytes of memory usage when running with the race detector. To
avoid having to reason about whether anything would ever access a nil
TxGroup, or run through iteratively zeroing maps, we just make a new
empty group at that point.
In [SUP-75](https://molecula.atlassian.net/browse/SUP-75?atlOrigin=eyJpIjoiYmU5MzdkMmUyZTAyNGQ2Y2IzMDMzYTgzMDU2Y2ZhNmMiLCJwIjoiaiJ9) Allen
pointed out that the time estimation is really good for the first couple lines of output, but gets exponentially worse as execution continues.
After looking into it, it looks like we’re currently using a heuristic based on the amount of messages processed in the previous
second(ish) which is what results in that sort of exponential drop off.
To remedy this, I adjusted the time estimation calculation to use the average time per message up to the point of calculating the new
estimate to ideally improve estimates over time, with the trade-off of a potentially less accurate estimate to begin with.
If the inner function that handles the open of storage and cache
fails, we close the fragment. If we closeStorage() before that,
then we can try to close the storage again, which causes a panic
when we try to mark the generation as Done again.
I was going to set f.gen = nil after marking it done, but I'm
not feeling safe about that -- there's too many places where
we check things about f.gen, and it seems unsafe. The generation
code should be removed at some point, because it all exists
as a workaround for not having any way to detect when reads are
"done", because we didn't want to do something huge and intrusive,
like adding the Tx system and requiring transactions to get
closed.
Performance of tests on MacOS has been atrocious for a while, and
a lot of that is fsync, so we're trying to make that optional.
To test all of this, I modified RBF to panic if anything tried to
open an RBF database without disabling fsync, and ran the tests that
way, and tracked down the various places this could still happen.
There's a lot of places in our tree where we were creating
test holders which were not getting created with fsync disabled, which
results in a surprisingly large number of points at which we end
up calling fsync in tests, which makes tests much slower than they
need to be. There's also a bunch of places where the flags don't get
propagated correctly; for instance, storage.fsync didn't propagate
to the RBFConfig.
We add an "fsync enabled" flag to OpenTranslateStoreFunc, so we can
tell translation stores that we don't need syncing, so the server's
config can be passed on appropriately.
More of the test code that sets things up is correctly configuring
that flag by default.
We also change the barely-used bolt storage backend to support this as
well.
With this done, the only calls to fsync left in a run of `go test -short`
in the top-level directory are from the zap logger in etcd, and consumed
around 0.03 seconds. The overall impact is that `go test -short`
went from "takes enough more than 10 minutes that i don't know how long
it takes" to about 2.5 minutes.
boltdb has a couple of places where it fsyncs even when fsync is
disabled, this turns out to cost an amazing amount of time over
several thousand databases in our test run. In theory, they are
rare circumstances compared to updates; in practice, when you
open 256 partition key translation databases per server opened
and most of them never get written to, not so much.
In test runs, we open, and close, *huge* numbers of databases. Even
the single fsync on close for these ends up being expensive on some
hosts. *cough* Apple. At least in theory, writes delivered to the
disk are just as written whether or not you've hit fsync, as long
as the machine doesn't power off before getting to them. In the
circumstances where we disable fsync, that's fine.
Since we already have an fsync function for "fsync if it's
not disabled", use that.
We disable fsync more consistently in testing, including using
etcd's already-existing UnsafeNoFsync option to disable fsyncs
in the backing store boltdb used by etcd, to reduce runtime of
our tests on MacOS significantly.
Corresponding to this, we update etcd by one patch to pick
up a locally-invented patch which turns out to be nearly-identical
to the upstream fix for "disabling fsync makes boltdb not
even bother to write some data sometimes", which caused crashes
galore.
This adds the following test:
1. cluster comes up (node 1,2,3), status normal
2. Pause node 3
3. Insert keys making sure to filter out the keys that will go to the paused node
4. Wait for status to become degraded
5. Unpause node 3
6. Wait for status to get back to normal
7. Check that keys were replicated to all 3 nodes
The actual code here is mostly jaffee's, but I've reworked it some.
This doesn't directly seem to be using UnionInPlace, but really it
is.
The actual logic inside (*Row).Union is a mess and probably silly
in a few ways, but hardly matters. The important part is that,
instead of calling it once per child as we get them, we gather
all of them at once and then call it on all of them. That gets
us a call to (*Row).Union that does a very elaborate dance to
compute a call to (*rowSegment).Union on the only segment present
in each of those rows, which then does a simpler thing to
call (*Bitmap).Union() with the first response as a receiver
and the rest as parameters, and THAT then ends up calling either
unionIntoTargetSingle() if there's only one other bitmap,
or using UnionInPlace on a Freeze() of the first bitmap, which
gets us (we hope) the benefits of the fancy UnionInPlace logic.
Every part of this is a reminder that we really need to replace
roaring and also the Row/rowSegment stuff some day.
I assumed the existing import code handled replicas. It doesn't, actually.
It just assumes they're handled. So, in the new import code, when splitting
things up by-shard, send each shard's data to *every* node that has
that shard, not just the first one.
It was useful having this in the package to verify code coverage of
the translator, but that having been verified, I'd sort of rather have
it NOT live in the package at all, it's really a testing-only kind
of thing.
We add a new protobuf type. Also, protoc changed slightly and remade
some tests, in a way which should have no effects but makes the code
*very* slightly cleaner.
This introduces the first testing code in encoding/proto (whoops)
so that scaffolding is a first draft; if you're looking at this code
and the design is a problem go ahead and fix it.
The purpose of this is to verify that we're actually covering all
the branches in the ingest.ShardedRequest and pb.ShardedIngestRequest
message conversions. (Except the top-level one for a nil request,
which isn't checked by this.)
The coverage report doesn't actually include coverage for the ingest
code, though, so we haven't actually properly tested Compare.
Baby steps!
We add endpoints and protobuf encode/decode to allow for sending
sharded requests over the wire in protobuf, so we can take our
sharded data and send it to other nodes if needed.
This is a squash of >15 other commits, so a bit of history
is relevant:
The Request type had FieldTypes in it because the field type
information was needed for sharding because sorting requires
that information. We change this around to make the external
sharding operation require the field types, and curry that
through the codec -- the codec is needed to tell the request
how it shards. (This is because the correct sorting order
varies by field type.) Requests (and ShardedRequests) no
longer have that table in them.
And then we hit a nasty bug in production and RCA showed
that our testing wasn't good enough and we need to be more
careful, and I discovered that test coverage in this package
was around 70%.
So, the other big thing here is coverage testing; in order to
make coverage testing viable and programmatically testable,
we have added the ability to render requests *back* to
JSON. This is not a great idea, but it does allow us to do
a lot of sanity-checking and verify that the encodings we're
using are consistent and correct.
This, plus some specific tests of decoding specific flawed
inputs, has caught a number of issues. Which are now fixed!
A lot of internal API surface got slightly changed, in ways
that make it simpler to work with. For instance, the
(*FieldOperation).TranslateUnsigned function doesn't really
need to exist; we can just have a non-method translate
function for unsigned and for signed, and use them based on
field type.
The stable translation hack used for testing had a bug that
could allow it to end up producing incorrect results if you
asked it to translate an ID first rather than exclusively
asking it to translate strings first, this has been
corrected. (This is a bug fix in code that was added
partway through creating this, but is tricky enough to
mention its own comment.)
Test coverage is now just over 90%, and a lot of what's left
is error-check returns that may well be actually unreachable
unless, say, the documentation for encoding/json is full of
lies. Which it probably is.
The view.go change is straightforward and fairly obviously more
correct.
The field.go change avoids holding the field read lock for the
duration of the mutex check request. The thinking was that while the
read lock was held something else was attempting to get a write lock,
which blocked all other read locks and something was getting into a
loop. Seebs might have a more detailed explanation, but that's as far
as my understanding goes at the moment. I believe this change is safe
though as we don't read/modify any field level data structures after
grabbing the standard view.
This takes our reasonably broad selection of predefined container
types and tries intersectionCallback on each pair of them, comparing
results against the results of plain old intersect(). We've had
several intersectionCallback fixes recently; every one of them
produces test failures here if reverted or broken, so I have at
least some confidence in this coverage.
Similarly, test everything on containerCallback, verifying that
we get the same set of values called back that we get from Slice().
Both of these were verified with -coverprofile to actually be
hitting all the lines of code that aren't insane edge case
checks like "what if a run is in the wrong order".
The inner loop of intersectionCallbackArrayArray's "fast"
case has
for len(ca) > 0 && ca[0] < va {
}
so we do not leave that loop unless len(ca) is 0, or
ca[0] >= va.
We then return from the whole function if len(ca) is 0,
so the only way we finish one iteration of the outer for
loop is if ca[0] >= va. Thus, this can be an `if` rather
than a `for`.
We also fix the logic for ArrayRun to make it require fewer
tests and be clearer about why the tests work and clearer about
always making progress.
And, finally, the bitmap/range callback logic, and the underlying
"callback per bit in word" logic, were both badly broken. In
particular, if a range started and ended in the same word, it would
hit the values in that word twice, once with them incorrectly
shifted, but then it would further garble any offsets past the first
in a word. Eww.
The merge lists behavior was flawed in that it would drop one item
from the list per merge, which means that, with high replication
and low number of distinct items, it could even produce an empty
list.
The actual "is there anything wrong" logic is fine, but the list of
clashing values set for a given record is not.
Unfortunately this also doubles the time the test takes, to
21 seconds on MacOS. OW.
It's hard to do this remotely sanely for the fragments, but the
translation and collation process itself could be fairly slow on
large data sets, so we should check occasionally for canceled
context and return early if no one needs the result anyway.
Also, take out no-longer-correct comments from the test case.
We support query parameters for details (default false) which
request additional data, and for a limit (default 0/MaxInt32)
on number of results returned to limit the amount of spam
produced if there's a lot of results. The simpler default
output should reduce load and runtime significantly, and the
ability to specify limits makes it easier to get reasonably
small responses.
There's some context support here, but the underlying filters
don't take contexts or check for them, which is probably
a flaw but might be a bit large to correct for this.
Despite being large, this set of changes is actually
fairly well contained within the mutex-checking code.
The rbf_bolt tests are unusually expensive, partially because they're
run with the race detector on, but also because it's basically running
two copies of all the tests and comparing them... But they haven't
detected anything in ages, because the RBF stuff is now pretty stable,
and those tests take about twice as long as anything else in our testing,
and thus impede our workflow noticably for little-to-no return. We might
some day want to fully remove them, but for now, just taking them out of
CI should streamline our workflows a bit.
Mutex fixes -- these address a couple of cases in which mutexes could end up with duplicate values, and also improve the testing so they're more likely to get caught.
This implements a fairly straightforward sanity-check for mutexes,
implemented as a bitmapfilter at the fragment level, and with higher
levels combining results. There's two endpoints, an internal endpoint
which only checks the local node's shards, and an external one which
forwards requests (using the internal endpoint) to all the other nodes.
The internal endpoint does not do key translation, the external one
does.
The transmission format is a probably-inefficient JSON blob, and
returns data separated per-shard so we don't have as much merging
work to do.
This introduces a horrifying monstrosity function which tries to
sneakily corrupt mutex fields and which has to be exported (EWWWWW)
but which is only present in _test code (!??!! THIS WORKS WHY).
Also one typo fix in unrelated code caused by not wanting to keep
fighting with gofmt about this.
Two of the intersectionCallback functions were broken.
In intersectionCallbackArrayArray, when checking to see whether we can
skip ahead 8, we need to check whether that last value is lower than
the one we're looking for, not whether the first value is.
For intersectionCallbackArrayBitmap, actually implement it at all;
it had never gotten modified significantly from the original
intersectionCount, so it still counted and returned intersections, but
never called the callback at all.
When doing the import tests, import all the data sets if there's
multiple data sets, and check that we're producing the correct number of
results including overwriting previous values, not just that we produce
the same number of values that we set, which shouldn't happen if there's
any overlap.
Also add a specific test that triggers the case I first ran into this for.
This is a design to let us write test cases for ingest with schema setup
and data in the json formats we want to use, and results as alternating
queries and expected results, so we can just create new test files and
run the tests against them. We also have to report back what we created
when creating things.
In the process of developing this, I noticed that the documentation describes
ingest schema as allowing more than one schema operation, but we didn't support
this, and also it wouldn't do much good because there was no way to do partial
things like "just add a field". Fixed.
Also we implement comparison for ops, so the test output is actually
a test rather than just some data to visually eyeball.
In the process, realize that the handling of timestamps was wrong; we said that we
take them as raw numbers relative to the epoch, not as raw Unix timestamps.
Also a couple of related cleanups caught by doing the testing.
This is a rework of Nia's radix sort. Still using stdlib sort for the
tail ends of things, and should probably replace it at some point
because it's still woefully inefficient, but this gets decent
performance, and lets us do the fancy thing of doing quick partial
sorting by record-key-only to get to shards, then deciding whether
to sort by value-then-record (as for a set field) or just by record
(as for int fields), which lets us reduce the amount of re-sorting
the same data by different criteria we do.
We also use a messy code-duplication basically-bubblesort for the
inner loops because it's much cheaper for small N.
This also lets us use field-aware sorting for shards, sorting them
correctly for a corresponding field type, and add corresponding API
support and fragment support for an option to tell the fragment
code that we already ordered things in the order that's most
efficient there, to avoid a second sort that we don't otherwise
need.
This partially-implemented prototype of the ingest API is based on our
programmatic ingest API reference. It has noticable limitations, most
crucially that it doesn't handle multi-node clusters right now. However,
it basically implements the expected semantics.
There's some noticeable performance issues to do with the high overhead
of sorting bits in order to import them efficiently, but this is fixable.
We also add the hooks to the internal client, and make the finisher logic
a bit smarter.
Much of this code was originally by Nia Weiss, but it's been merged
and restructured a bit to get things broken into logical commits.
This is sort of horrible, but viewsByTime was about 25% of total CPU time in
the ingest path, NOT including increased GC overhead. This overoptimized
approach to letting us recycle a buffer, and use the same buffer for multiple
time views at once, reduces that to about 2.5%. Sorry for the mess.
We also streamline the process of building the per-view data sets a bit,
and streamline it a lot in the non-time-quantum case.
In fact, we have a number of things assuming that values passed to Import
always fit within a single known shard, so, drop all the extra complexity
around this, drop the computation of fancy view/shard keys, and so on.
There's a lot of room left to improve this probably but it's at least
better, I think.
Unfortunately, there's a handful of things, basically all of which are
test cases, which were relying on this, so, we also add functionality
for splitting import requests by shards. But this allows us to stop
duplicating each shard's inputs one at a time... which turns out to
mean that we now care that the import operation can write back to the
import request. This only affects test cases, so we adopt a crufty
hack involving cloning import requests in those rare cases, and also
when reusing the same column IDs to write to the existence field that
we'd be using later to write to another field.
Note that even if we weren't overwriting the column IDs with positions,
we'd be sorting the column/row ID lists by row-then-column, which means
we'd still be corrupting the column ID lists. This may want to change
at some point.
We also reuse a single Tx for all the views, because DB-per-shard
means that should work fine, and reduces the cost of doing these
updates, probably.
staticcheck points out that the break is otherwise an ineffective
break because it just ends the current case clause of the switch
it's in, which is true.
The convention of using a "u" for "micro" is pretty well-established and some
people will have trouble typing the Greek letter, accept that as a synonym.
We're reading timestamps as []int64, instead of allocating a time.Time
for each timestamp, just use the same logic to determine whether to use the
int64 timestamp that we would have used to decide whether to allocate it.
We still have to check the whole run, though, because we're providing a large
list of 0s instead of "no timestamps", for Reasons.
A nil Qcx is a crime against existence and makes baby pandas cry.
Having taken out the hack that tried to accommodate this when tests did it,
we now have to fix the tests. Oh no.
This commit adds a `Future` scope to the configuration options, and for
the time being includes a single flag within that scope: `rename`.
Usage:
--future.rename
The value is a boolean available internally at: m.Config.Future.Rename
It's not enough to back up each index's translation keys after
backing up that index's data; we also have to back them up after
backing up any index data from indexes which have foreign key
references to that index. So we do the per-index passes separately.
Since the individual backup data files are being created separately,
the expected output is unchanged for a quiescent database, the only
difference is that the amount of translation info which might be
newer than the data stored for shards is potentially increased.
Previously the backup tool only fsync'ed the files.
Since the directories were not synced, it was possible for the references to be lost.
Now we sync the entire output directory tree and its parent.
If we explicitly shut a node down, we don't want everyone else
thinking it's up for the next 5 seconds. Worse, in CI, we have random
long delays (10+ seconds) with no CPU activity at all, so we have to
set the TTL longer there. Which makes any test checking for responsive
detection of a node going down take even longer. So! We revoke
leases on our way down, and this makes the tests not take so
long, and allows us to have a reasonable timeout on the test, while
having a completely unreasonable HeartbeatTTL to make CI stop
breaking randomly.
After continuing to see weird test failures, did some more careful testing,
discovered that CI can pause a machine entirely for up to 29 seconds
or so very rarely, and 5-10 seconds quite frequently, which causes
cascading heartbeat failures and so on. Remove those.
CircleCI is providing a ramdisk as /mnt/ramdisk. Using the ramdisk
instead of local storage makes fsync operations essentially free,
which removes (some of) the frequent multi-second delays we see during
runs otherwise.
It turns out that it's desireable to be able to configure the bootstrap
timeout for etcd, because during startup, we end up delaying that long
(N-1) times in series during each cluster creation, which is pointless
when we're starting the whole cluster. Reduces test runtime by several
minutes.
Time fields were not listed as a type which could accept key translation, causing the translation code to fail.
This also adds tests for keyed time and mutex fields.
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 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'
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.
* [FeatureBase Community Help](https://github.com/FeatureBaseDB/FB-community-help)
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.
Follow along with the [Sample Project](https://internal-docs.molecula.cloud/tutorials/getting-started) to get a better understanding of FeatureBase's capabilities.
## Pilosa is now FeatureBase
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.)
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].
## 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)
### Build FeatureBase Server from source
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.
### Data Model
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)
### Ingest Data and Query
* [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)
## Community
You can email us at community@featurebase.com and [learn more about contributing](https://github.com/FeatureBaseDB/featurebase/blob/master/OPENSOURCE.md).
Chat with us: [https://discord.gg/FBn2vEp7Na][Discord]
## 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.
* 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.
## License
FeatureBase is licensed under the [Apache License, Version 2.0][License]
// 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."`
"Id", "Name", "Short description", "Gender", "Country", "Occupation", "Birth year", "Death year", "Manner of death", "Age of death"
1, "George Washington", "1st president of the United States (1732–1799)", "Male", "United States of America; Kingdom of Great Britain", "Politician", "1732", "1799", "natural causes", "67"
3, "Abraham Lincoln", "16th president of the United States (1809-1865)", "Male", "United States of America", "Politician", "1809", "1865", "homicide", "56"
4, "Wolfgang Amadeus Mozart", "Austrian composer of the Classical period", "Male", "Archduchy of Austria; Archbishopric of Salzburg", "Artist", "1756", "1791", "0", "35"
5, "Ludwig van Beethoven", "German classical and romantic composer", "Male", "Holy Roman Empire; Austrian Empire", "Artist", "1770", "1827", "0", "57"
6, "Jean-François Champollion", "French classical scholar", "Male", "Kingdom of France; First French Empire", "Egyptologist", "1790", "1832", "natural causes", "42"
// TODO(tlt): we can't run this test until we get the system tables under control (i.e. sorted). Currently, fb_views is in a map with users, so the following can fail 50% of the time.
// Show tables for database by calling describe with no args.
EXPECT:| 1 | George Washington | 1st president of the United States (1732–1799) | Male | United States of America; Kingdom of Great Britain | Politician | 1732 | 1799 | natural causes | 67 |
EXPECT:| 2 | Douglas Adams | English writer and humorist | Male | United Kingdom | Artist | 1952 | 2001 | natural causes | 49 |
EXPECT:| 3 | Abraham Lincoln | 16th president of the United States (1809-1865) | Male | United States of America | Politician | 1809 | 1865 | homicide | 56 |
EXPECT:| 4 | Wolfgang Amadeus Mozart | Austrian composer of the Classical period | Male | Archduchy of Austria; Archbishopric of Salzburg | Artist | 1756 | 1791 | 0 | 35 |
EXPECT:| 5 | Ludwig van Beethoven | German classical and romantic composer | Male | Holy Roman Empire; Austrian Empire | Artist | 1770 | 1827 | 0 | 57 |
EXPECT:| 6 | Jean-François Champollion | French classical scholar | Male | Kingdom of France; First French Empire | Egyptologist | 1790 | 1832 | natural causes | 42 |
EXPECT:| 7 | Paul Morand | French writer | Male | France | Artist | 1888 | 1976 | 0 | 88 |
EXPECT:| 8 | Claude Monet | French impressionist painter (1840-1926) | Male | France | Artist | 1840 | 1926 | natural causes | 86 |
