Persisting Introspection Dataflows Part 1 #13340

lluki · 2022-06-29T07:52:16Z

This PR adds the first steps for sinking the introspection source to persist. It is an updated version of Jan's PoC PR #13236 .

The introspection sources are exposed in mz_catalog with the postfix of the replica. The default cluster creates a replica with one id 1. Thus for example the query

SELECT * FROM mz_catalog.mz_dataflow_operators_1;

will return the data of the default cluster. Newly created replicas will create the corresponding catalog entries as can be checked with \dt mz_catalog.*; in psql.

The introspection shards are stored with the replica data in the stash/catalog. Thus on restart the same shards are re-used. Allowing an external reader to obtain an uninterrupted stream of updates. The computed's will remove stale date upon start.

As of now, these sources are kept after a DROP replica/cluster, there is an outstanding design discussion on how to handle this case.

Motivation

This PR adds a known-desirable feature.

Related to [Epic] Introspection sources/views across replicas #11782 (see TODOs on what's missing)

Testing

Basic testing ("create cluster creates a new log entry") has been added in test/sqllogictest/cluster_log_sinks.slt. Existing tests verify the presence of introspection sources for the default cluster.

Release notes

This PR includes the following user-facing behavior changes:

Introspection sources are now per-replica. Instead of SELECT * FROM mz_catalog.mz_dataflow_operators; the introspection source should be post-fixed with the replica id (such as SELECT * FROM mz_catalog.mz_dataflow_operators_1;).

antiguru

This looks mostly fine, but it'd be great to know why we cannot read the data from the persist source. I think we should either have a convincing answer as to what causes the problem or make sure it works.

I left some minor comments. In general, it'd be great to have more doc comments, even if things aren't pub.

antiguru · 2022-06-29T08:12:37Z

src/compute/src/render/sinks.rs

+use mz_dataflow_types::client::controller::storage::CollectionMetadata;
 use timely::dataflow::Scope;

-use mz_dataflow_types::client::controller::storage::CollectionMetadata;
 use mz_dataflow_types::sinks::{SinkConnection, SinkDesc, SinkEnvelope};


src/dataflow-types/src/logging.proto

teskje · 2022-06-29T10:06:07Z

I quickly tried this locally. Two things I noticed:

There seem to be no introspection sources created for the default replica. Is this another TODO?

When I do this:

CREATE CLUSTER c1 REPLICAS (r (SIZE '1'));
SET cluster = c1;
CREATE TABLE foo (a int, b int);
CREATE MATERIALIZED VIEW v AS SELECT a + b FROM foo;
SELECT * FROM mz_dataflow_operators_2;

... computed crashes with:

thread 'timely:work-0' panicked at 'index out of bounds: the len is 2 but the index is 2', src/compute/src/render/context.rs:612:69

Could that be the reason why no introspection data is returned?

antiguru · 2022-06-29T11:02:20Z

I can't reproduce the crash -- can you? This should never happen as it indicates malformed rows, or the knowledge of row formats being incorrect.

lluki · 2022-06-29T11:24:16Z

I'll try this once i merged upstream. it seems the storage controller changed a bit...

Default cluster is definetly a TODO!

teskje · 2022-06-29T11:35:26Z

I can reproduce it, but sometimes I need to repeat the final SELECT a couple times.

lluki · 2022-06-29T11:50:25Z

I can also reproduce the issue (with a couple of tries). Good news is, computed crashes, gets restarted and then the query goes through 😲

antiguru · 2022-06-29T11:58:55Z

I can't reproduce the crash -- can you? This should never happen as it indicates malformed rows, or the knowledge of row formats being incorrect.

I forgot to uncomment the code to enable logging to persist! Now it repros nicely. A good candidate to fix before merging :)

* Refactors the logging dataflows such that they produce an unarranged version * sink_logs instructs the computed to send these unarranged dataflows to the indicated persist shard.

src/adapter/src/catalog.rs

src/adapter/src/coord.rs

antiguru

Thanks for this effort! I finished a first round of review. It looks like it's headed in the right direction, but I wanted to use the opportunity to add some comments to improve the quality/reduce the diff size. Specifically, some changes seem to be stray, and while they work, they're not needed.

The only thing I'd change are the changes to reachability.rs where we now potentially do duplicate work.

src/adapter/src/catalog.rs

src/adapter/src/coord.rs

antiguru · 2022-07-07T13:23:05Z

src/adapter/src/coord.rs

+        let instance = self.catalog.resolve_compute_instance(&of_cluster)?.clone();
        let replica_id = instance.replica_id_by_name[&name];
+        let replica = instance.replicas_by_id[&replica_id].clone();
+
+        if let Some(c) = &instance.logging {
+            self.initialize_compute_read_policies(
+                introspection_collection_ids,
+                instance.id(),
+                Some((c.granularity_ns / 1000) as u64),
+            )
+            .await;
+        }
+
        self.dataflow_client
-            .add_replica_to_instance(instance.id, replica_id, config)
+            .add_replica_to_instance(
+                instance.id,
+                replica_id,
+                replica.config,
+                replica.log_collections,
+            )


You can avoid the clone by capturing instance.id() in a local variable. Also, you're using instance.id() in the first call and instance.id in the second, is this intended?

Yes.

Another question, I have a double self.catalog.resolve_compute_instance(&of_cluster)?; is there a good way to get rid of this? The problem is that self.catalog_transact needs &mut self, and i need instance to go out of scope before. So I don't see an obvious way (code coming in a sec)

src/compute/src/logging/compute.rs

src/compute/src/logging/reachability.rs

teskje

I've added some comments, but these are mostly smaller things. I need to look at this again when I'm more awake :/

src/compute-client/src/controller/replicated.rs

src/compute-client/src/logging.proto

src/compute-client/src/logging.rs

src/adapter/Cargo.toml

src/compute/src/logging/compute.rs

src/compute/src/logging/persist.rs

src/compute/src/logging/reachability.rs

src/compute/src/logging/timely.rs

Co-authored-by: Moritz Hoffmann <[email protected]>

maddyblue · 2022-07-11T16:37:42Z

introspection source with the replica id as postfix

Is there a design justification for this? This is an anti pattern for normal SQL applications, and it would be bad form for a sql database itself to implement this pattern unless there was a very good reason. The replica id should be a column, not a postfix of the name. If the reason is "our current code makes this hard" then we should fix that instead of ship it to users.

I think this is one of those things where our non-cloud roots are showing their age and we need a way to fix it. It seems likely that someone on the adapter side might need to work with you to do something here, but as an adapter person I'm not sure what because I don't have a clear understanding of how these introspection sources work.

src/adapter/src/catalog.rs

danhhz

I don't understand 100% of the context in which this is used, but I left comments about persist usage, as requested

I do have a larger thought that it'd be nice to stop adding bespoke persist sink operators, we already have 1 fork of it. (e.g. we're about to do some work to increase the performance of persist_sink and it's gonna be a pain to have to keep all these maintained.) better, if we can swing it, is something like the pattern I linked in slack, where we represent as dataflow Collections "this is what I want in persist" and "this is what is in persist" and some common code just does what is necessary to make that happen. unclear to me if it can be used here, but we should definitely think about it. https://materializeinc.slack.com/archives/C03K23ECB8U/p1658248877515799?thread_ts=1658241816.436409&cid=C03K23ECB8U

src/compute/src/logging/persist.rs

antiguru

This looks like things are coming together, thanks for the progress! I left a few comments, please address them before merging.

The PR currently has tests, but it seems there are no tests that read from the new variants (e.g., mz_arrangement_batch_internal_1). I think it would be good to have some test that these sources (1) produce data, and (2) while the arranged data still exists, they produce the same data.

src/adapter/src/catalog/storage.rs

src/compute-client/src/controller.rs

src/compute/src/logging/reachability.rs

active_logs have moved to btreemap, do the same to persisted_logs

teskje

LGTM, considering that you've mentioned that things like cleaning up introspection sources, removing the arrangements, and removing the persist_sink code duplication are TODOs for later.

My comments are mostly nits, but I agree with @antiguru that having tests that actually read from the new introspection sources would be good.

src/adapter/Cargo.toml

src/compute-client/src/controller/replicated.rs

src/adapter/src/catalog/storage.rs

src/compute-client/src/logging.rs

src/compute/src/logging/differential.rs

src/compute/src/logging/reachability.rs

lluki · 2022-07-21T14:23:37Z

Adressed nits & added a test that diff's to the active logs. Will hit merge+squash if tests are green!

jkosh44

Migration LGTM

lluki requested review from teskje and antiguru June 29, 2022 07:53

antiguru reviewed Jun 29, 2022

View reviewed changes

teskje mentioned this pull request Jun 29, 2022

Internal sinks, part 3: coordinator plumbing #13346

Merged

1 task

lluki force-pushed the internal-sinks-poc branch 3 times, most recently from 4ac7fd2 to a7bdedd Compare July 7, 2022 09:02

lluki force-pushed the internal-sinks-poc branch from c010b71 to bb0b33c Compare July 7, 2022 12:45

lluki requested a review from antiguru July 7, 2022 12:56

lluki added 2 commits July 7, 2022 15:02

Add and respect sink_logs to CreateInstance

08418e6

* Refactors the logging dataflows such that they produce an unarranged version * sink_logs instructs the computed to send these unarranged dataflows to the indicated persist shard.

Make coordinator use sink_logs

e551941

lluki force-pushed the internal-sinks-poc branch from bb0b33c to e551941 Compare July 7, 2022 13:08

lluki commented Jul 7, 2022

View reviewed changes

src/adapter/src/catalog.rs Outdated Show resolved Hide resolved

lluki changed the title ~~Internal sinks poc~~ Persisting Introspection Dataflows Part 1 Jul 7, 2022

lluki commented Jul 7, 2022

View reviewed changes

src/adapter/src/coord.rs Outdated Show resolved Hide resolved

antiguru requested changes Jul 7, 2022

View reviewed changes

teskje reviewed Jul 7, 2022

View reviewed changes

lluki and others added 6 commits July 7, 2022 17:36

Update src/adapter/src/catalog.rs

5e95b40

Co-authored-by: Moritz Hoffmann <[email protected]>

Addressing small PR comments

2755146

Make specialize_command a method of Replica

14ef097

Merge remote-tracking branch 'upstream/main' into internal-sinks-poc

7da3f29

fixup log source registration

90303f4

add basic test for sinked logs

bf3f568

lluki added 3 commits July 19, 2022 15:45

Correctly initialize storage policies on bootstrap

acc1232

add new catalog entries to testsuite

dacdf31

small fixes

b38d7d6

lluki marked this pull request as ready for review July 19, 2022 17:33

lluki requested a review from antiguru July 19, 2022 17:34

maddyblue reviewed Jul 19, 2022

View reviewed changes

src/adapter/src/catalog.rs Outdated Show resolved Hide resolved

src/adapter/src/catalog.rs Show resolved Hide resolved

danhhz reviewed Jul 19, 2022

View reviewed changes

benesch mentioned this pull request Jul 19, 2022

adapter: support for dynamic unions #13743

Open

lluki added 5 commits July 20, 2022 15:36

use install_desired_into_persist for persisting logging

864aa25

remove future-executor

127d15d

clippy fixes

b0daeec

small fixes

004f610

force writer to be node 0

802b19d

antiguru reviewed Jul 21, 2022

View reviewed changes

src/adapter/src/catalog/storage.rs Outdated Show resolved Hide resolved

src/compute-client/src/controller.rs Outdated Show resolved Hide resolved

src/compute/src/logging/reachability.rs Outdated Show resolved Hide resolved

lluki added 4 commits July 21, 2022 13:22

persisted_logs: HashMap -> BTreeMap

9e8a9fc

active_logs have moved to btreemap, do the same to persisted_logs

merge in frank's latest update

fda8c7b

reachability: only create one common dataflow

d42a09b

clippy fix

4783ac5

teskje reviewed Jul 21, 2022

View reviewed changes

antiguru approved these changes Jul 21, 2022

View reviewed changes

lluki added 2 commits July 21, 2022 16:10

add test that diffs to active introspection

3bd1452

adressing nits and clippy

ca6e199

lluki added 3 commits July 21, 2022 16:24

more nits

29452b3

Merge remote-tracking branch 'upstream/main' into internal-sinks-poc

83ad8f7

add datum to version

5f7584d

jkosh44 approved these changes Jul 21, 2022

View reviewed changes

teskje approved these changes Jul 21, 2022

View reviewed changes

lluki merged commit c8c6947 into MaterializeInc:main Jul 21, 2022

umanwizard mentioned this pull request Jul 26, 2022

CREATE CLUSTER REPLICA takes several seconds to return #13854

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persisting Introspection Dataflows Part 1 #13340

Persisting Introspection Dataflows Part 1 #13340

lluki commented Jun 29, 2022 •

edited

Loading

antiguru left a comment

antiguru Jun 29, 2022

teskje commented Jun 29, 2022

antiguru commented Jun 29, 2022

lluki commented Jun 29, 2022

teskje commented Jun 29, 2022

lluki commented Jun 29, 2022

antiguru commented Jun 29, 2022

antiguru left a comment

antiguru Jul 7, 2022

lluki Jul 7, 2022

teskje left a comment

maddyblue commented Jul 11, 2022

danhhz left a comment

antiguru left a comment

teskje left a comment

lluki commented Jul 21, 2022

jkosh44 left a comment

Persisting Introspection Dataflows Part 1 #13340

Persisting Introspection Dataflows Part 1 #13340

Conversation

lluki commented Jun 29, 2022 • edited Loading

Motivation

Testing

Release notes

antiguru left a comment

Choose a reason for hiding this comment

antiguru Jun 29, 2022

Choose a reason for hiding this comment

teskje commented Jun 29, 2022

antiguru commented Jun 29, 2022

lluki commented Jun 29, 2022

teskje commented Jun 29, 2022

lluki commented Jun 29, 2022

antiguru commented Jun 29, 2022

antiguru left a comment

Choose a reason for hiding this comment

antiguru Jul 7, 2022

Choose a reason for hiding this comment

lluki Jul 7, 2022

Choose a reason for hiding this comment

teskje left a comment

Choose a reason for hiding this comment

maddyblue commented Jul 11, 2022

danhhz left a comment

Choose a reason for hiding this comment

antiguru left a comment

Choose a reason for hiding this comment

teskje left a comment

Choose a reason for hiding this comment

lluki commented Jul 21, 2022

jkosh44 left a comment

Choose a reason for hiding this comment

lluki commented Jun 29, 2022 •

edited

Loading