* 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
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.
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.
* 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>
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.
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)
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.
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.
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.
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
```
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.
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.
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.
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.
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.
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.
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.
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.
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.
