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.
