* the metric attributes are hardcoded to the path
* the duration (frequency) of the background task is hardcoded
* the tick.await now occurs after the first metric recording, such that the test doesn't have to wait 15 seconds.
This implements `PersistCompletionObserver` for the `WalReferenceHandle`
so that it can be given to the persist handle and notified of persist
completions in order to drop WAL segments once all writes are
persistent.
This commit updates the DML sink for the write-ahead log to notify the
reference tracker of writes that have been committed to the log, but
failed to be applied to the buffer tree.
This is the first commit in line to connect the WAL segment reference
tracker actor up to the rest of the ingester. It removes the segment file
deletion and hacky sleep from the rotate task, deferring to the actor
for deletion tracking.
Writes now contain multiple sequence numbers, so the WAL reference
actor must be notified of *all* sequence numbers contained for a write
that failed to be applied to the buffer.
This adds extra test coverage for the ingester's WAL replay & RPC write
paths, as well as the WAL E2E tests, to ensure that all sequence numbers
present in a WriteOperation/WalOperation are encoded and present when
decoded.
This commit asks the oracle for a new sequence number for each table
batch of a write operation (and thus each partition of a write) when
handling an RPC write operation before appending the operation to the
WAL. The ingester now honours the sequence numbers per-partition when
WAL replay is performed.
This commit removes the op-level sequence number from the proto
definition, now reading and writing solely to the per table (and thus
per partition) sequence number map. Tables/partitions within the same
write op are still assigned the same number for now, so there should be
no semantic different
Prior to this change projection pushdown was implemented as a filter,
which meant a query using it would take the following steps:
* Query arrives
* Find necessary partition data
* Copy all the partition data into a RecordBatch
* Filter that RecordBatch to apply the projection
* Return results to caller
This is far from ideal, as the underlying partition data is copied in
its entirety and then the unneeded columns discarded - a pure waste!
After this PR, the projection is pushed down to the point of RecordBatch
generation:
* Query arrives
* Find necessary partition data
* Copy only the projected columns to a RecordBatch
* Return results to the caller
This minimises the amount of data copying, which for large amounts of
data should lead to a meaningful performance improvement when querying
for a subset of columns. It also uses a slightly more efficient
projection implementation by using a single pass over the columns (still
O(n) but less constant overhead).
Configure the partition pruning test to use a partition template that
partitions on the "region" field. This will allow it to be used for
pruning at query time.
Similar to #8109.
This was once implemented by the RUB but as it stands right now, no
chunk implements this anymore.
If we ever want to bring this back, we should use the output of
`QueryChunk::data` instead (i.e. use a data-based implementation instead
of a per-chunk one).
Closes#8096.
This interface was once specially implemented by the RUB. The only
actual implementation of it is within the querier that just forwards it
to a simple schema scan. Lift this semantic to `iox_query_influxrpc`
instead so all the chunks can use it.
If we ever want to optimize this again, we should use `QueryChunk::data`
instead (i.e. instead of implementing it within the chunk it should use
the data method and do something smart based on that).
First half of #8096.
Do not (ab)use per-chunk delete predicates for the retention policy.
Instead use a per-table predicate.
This makes the code way cleaner, since the scoping is correct (i.e.
delete predicates are a table-wide attribute, not a chunk-based one) and
it is consistent time predicates that the user providers (e.g. via
`WHERE time > x`).
It also allows us to remove delete predicates (in their current,
non-scalable form) from the query path. A potential future version would
likely not use per chunk predicates (and "is processed" markers) but use
the timestamp / chunk order to determine to which data the predicate
should be applied.
Note that the lowering of the retention policy changed slightly from
```text
(time > (now() - retention)) AND (time < MAX)
```
to
```text
time > (now() - retention)
```
Since the `MAX` cut is just an artifact of the lowering and was unnecessary.
Closes#7409.
Closes#7410.
Moved TABLE2_ID and TABLE2_NAME to the top of the test module, even
though TABLE2_NAME is only used in one spot, to encourage use of the
constants if new tests are added to this file that need a table that's
different from the arbitrary table.
Replaced all occurrences of TableId::new(1234) with TABLE2_ID even
though TABLE2_ID is 1234321; the exact value doesn't matter, the
important property is that it does not equal ARBITRARY_TABLE_ID (which
is 4).
Replace:
- PartitionId::new(0) with ARBITRARY_PARTITION_ID (which is actually 1)
- PartitionId::new(1) with PARTITION2_ID (actually 2)
- PartitionId::new(2) with PARTITION3_ID (actually 3)
So while adding one is a bit confusing in this diff, in the long run,
this will make the test more understandable and consistent with other
tests.
So that when I change the type of PartitionIds to TransitionPartitionId,
I don't have to update all these places that just need an arbitrary
partition ID or related values.
These test constants probably didn't exist when these tests
were created.
Now that sequence numbers are internal to the ingester and the WAL,
there's no need for them to be a signed integer. As noted by
[#7260](https://github.com/influxdata/influxdb_iox/issues/7260) this was
a quirk related to the kafka-based IOx and Postgres only supported
signed integers.
This will hold the deterministic ID for partitions.
Until all existing partitions have this value, this is optional/nullable.
The row ID still exists and is used as the main foreign key in the
parquet_file and skipped_compaction tables.
The hash_id has a unique index so that we can look up records based on
it (if it's available).
If the parquet file record has a partition_hash_id value, use that to
generate the object storage path instead of the partition_id.
This enables the whole of the RPC write path to use the new
`dml_payload::IngestOp` instead of the `DmlOperation` type. The
implementation of `From<&WriteOperation> for DmlWrite` has been removed
to complete the switch.
Removes one of the temporary conversion traits and adds a test helper
method `encode_batch(NamespaceId, WriteOperation)` for removal of the
`DmlOperation` from WAL replay and the RPC write handler.
This change replaces the test_util equality and write generation code
to use the new `IngestOp::Write(WriteOperation)` type, removing many
pointless conversions in tests.
This commit switches over the trait definition to take `IngestOp`
instead of `DmlOperation`. This commit is not enough to complete the
switch, as the conversion from `DmlOperation` and back is a performance
regression.
This commit implements the `From` trait to allow quick conversion from
a `&WriteOperation` back to an owned `DmlWrite`. This conversion
copies all the record batches and should be removed once WAL encoding
can be done from `&WriteOperation`.
This allows accessing temporarily agnostic values without matching on
the kind of `IngestOp`. There is only one type at the moment, so this
just provides a bridge for users of the `DmlOperation` getters.
This commit implements the `From` trait to allow quick conversion from
`DmlOperation::DmlWrite` to `IngestOp::WriteOperation`. This conversion
performs some copies and should be removed once the RPC write path has
been switched to use `IngestOp`.
* add FlightService metric to record a duration histogram across requests.
* duration per partition, per request
* make available to the FlightFrameEncodeRecorder
* update naming conventions to reflect updated functionality
The `dml` crate and its contained types simultaneously contain more and
less data than the ingester needs for writes. This type is to replace
the use of `DmlOperation` and `DmlWrite` within the ingester's internals
so that the type can be specialised with low blast-radius changes.
The key change here is to remove the ties to the `DmlMeta` construction
and allow sequencing of data on a per-partition basis
Nothing gets the partition ID out of the metadata. The parts of the code
interacting with object storage that need the ID to create the object
store path were using the partition ID from the metadata out of
convenience, but I changed those places to pass in the partition ID in a
separate argument instead.
This will make the transition to deterministic partition IDs a bit
smoother.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
This adds a little extra layer of type safety and should be optimised
by the compiler. This commit also makes sure the ingester's WAL sink
tests assert the behaviour for partitioned sequence numbering on an
operation that hits multiple tables & thus partitions.
Writes are partitioned before being placed in the buffer tree. This
has the effect of splitting up the persistence of a DmlWrite's contents
and thus the persistence of data referred to by write operations placed
into a single WAL entry for a write op.
This change associates the currently assigned sequence number
with every `TableId` in the write, so that persist events for a single
write can be tracked on a per table/partition level.
Making this partial change enables a transition period where changes
can be rolled back and WAL files can still be processed.
A future change will produce a new sequence number per table
ID.
This is the major part of #7470. Additional clean ups (e.g. to remove
the actual types from `data_types`) will follow.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
A short description on the FlightFrameEncodeRecorder that helps people
understand exactly what the spans cover - it's likely people will wind
up looking at this code after debugging an issue in a trace, so lets
make sure we give them as much helpful context as possible!
* test: integration test for tracing of queries to the ingester
* chore: add FlightFrameEncodeRecorder to record spans per each polling result
* refactor(trace): impl TraceCollector for Arc
Allow any Arc-wrapped TraceCollector implementation to be used as a
TraceCollector. This avoids needing to as_any() and downcast later.
* test: assert FlightFrameEncodeRecorder trace spans
This test exercises the FlightDataEncoder wrapped with the trace
decorator (FlightFrameEncodeRecorder) when executing against a data
source that yields data after varying numbers of Stream polls.
This test passing will validate the FlightFrameEncodeRecorder correctly
instruments the amount of time a client spends waiting on the
FlightDataEncoder to acquire or encode a protocol frame, but also
ensures the decorator correctly accounts for varying behaviours allowed
through the Stream abstraction. It does this by simulating a data source
that is not always immediately ready to provide data, such as a buffer
wrapped in a contended async mutex.
* refactor: move tracing decorator into separate mod
* fix: record spans
* refactor(test): update test
The frame encoder is not one-to-one - it emits two frames for the first
data payload, a schema and a payload. This commit updates the test to
account for it!
* refactor: remove unneeded mut ref, and use enum state method which panics when in a (should be unreachable) state
* chore: add more docs to FlightFrameEncodeRecorder and related
---------
Co-authored-by: Dom Dwyer <dom@itsallbroken.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* fix(ingester): re-transmit schema over flight if it changes
Fixes https://github.com/influxdata/idpe/issues/17408 .
So a `[Sendable]RecordBatchStream` contains `RecordBatch`es of the SAME
schema. When the ingester crafts a response for a specific partition,
this is also almost always the case however when there's a persist job
running (I think) it may have multiple snapshots for a partition. These
snapshots may have different schemas (since the ingester only creates
columns if the contain any data). Now the current implementation munches
all these snapshots into a single stream, and hands them over to arrow
flight which has a high-perf encode routine (i.e. it does not re-check
every single schema) so it sends the schema once and then sends the data
for every batch (the data only, schema data is NOT repeated). On the
receiver side (= querier) we decode that data and get confused why on
earth some batches have a different column count compared to the schema.
For the OG ingester I carefully crafted the response to ensure that we
do not run into this problem, but apparently a number of rewrites and
refactors broke that. So here is the fix:
- remove the stream that isn't really as stream (and cannot error)
- for each partition go over the `RecordBatch`es and chunk them
according to the schema (because this check is likely cheaper than
re-transmitting the schema for every `RecordBatch`)
- adjust a bunch of testing code to cope with this
* refactor: nicify code
* test: adjust test
This commit fixes loads of crates (47!) had unused dependencies, or
mis-configured dependencies (test deps as normal deps).
I added the "unused_crate_dependencies" to all crates to help prevent
this mess from growing again!
https://doc.rust-lang.org/beta/nightly-rustc/rustc_lint_defs/builtin/static.UNUSED_CRATE_DEPENDENCIES.html
This has the minor downside of false-positives when specifying
dev-dependencies for test/bench binaries - these are files in /test or
/benches (not normal tests). This commit includes a workaround,
importing them in lib.rs (gated by a feature flag). I think the
trade-off of better dependency management is worth it!
wiedld
Fraser Savage
Dom
kodiakhq[bot]
dependabot[bot]
Marco Neumann
Carol (Nichols || Goulding)
wiedld
Carol (Nichols || Goulding)
Andrew Lamb
Andrew Lamb