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