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