models: disable bindings using a boolean #1100

psFried · 2023-07-07T03:40:14Z

Description:

This changes the representation of disabled capture and materialization bindings in Flow specs. We previously used target: null and resource: null to represent disabled capture and materialization bindings respctively. That change was introduced quite recently and has not seen any use yet. This commit changes both capture and materialization bindings to use disabled: true.

Ultimately, the reason for this is to make it easier to programatically manipulate Flow specs. Say a user runs a discover of an existing capture via the UI. Afterward, the draft could contain a bunch of disabled bindings with target: null along with a corresponding collection for each disabled binding. The UI needs those collections so that it can make sure they're included in the draft if the user re-enables any of the disabled bindings. But the UI would have no idea how to match a disabled binding with its corresponding collection spec because the target is null. We'd have a similar problem in Flowctl. The output of a discover needs to somehow communicate the relationship between each binding and the collection that goes along with it. Rather than introduce a separate data structure containing these relationships, it seems preferable to just keep them within the specs themselves.

This ends up being a simpler, it works the same for both captures and materializations, and seems a bit more obvious and clear. It also means that disabled capture bindings would be required to have a non-null target. The validation logic is skipped for bindings that are disabled, so disabled bindings can have any collection name. It's a little awkward, but it seems preferable to the alternative of having a separate data structure containing the mappings from resources to collections.

Workflow steps:

To disable a capture binding, use:

captures:
  acmeCo/foo:
    bindings:
    - resource: { stream: foo }
      target: acmeCo/some/collection
      disable: true

and for materializations:

materializations:
  acmeCo/foo:
    bindings:
    - resource: { table: foo }
      source: acmeCo/some/collection
      disable: true

Documentation links affected:

None yet.

Notes for reviewers:

I want to explicitly call out that the premise of this PR is open for debate. There's tradeoffs here, where we're using a somewhat less elegant representation (due to having a required target for disabled bindings), in order to make it easier to programmatically manipulate the specs. We don't have to do this.

This change is

This changes the representation of disabled capture and materialization bindings in Flow specs. We previously used `target: null` and `resource: null` to represent disabled capture and materialization bindings respctively. That change was introduced quite recently and has not seen any use yet. This commit changes both capture and materialization bindings to use `disabled: true`. Ultimately, the reason for this is to make it easier to programatically manipulate Flow specs. Say a user runs a discover of an existing capture via the UI. Afterward, the draft could contain a bunch of disabled bindings with `target: null` along with a corresponding collection for each disabled binding. The UI needs those collections so that it can make sure they're included in the draft if the user re-enables any of the disabled bindings. But the UI would have no idea how to match a disabled binding with its corresponding collection spec because the `target` is null. We'd have a similar problem in Flowctl. The output of a discover needs to somehow communicate the relationship between each binding and the collection that goes along with it. Rather than introduce a separate data structure containing these relationships, it seems preferable to just keep them within the specs themselves. This ends up being a simpler, it works the same for both captures and materializations, and seems a bit more obvious and clear. It also means that disabled capture bindings would be required to have a non-null `target`. The validation logic is skipped for bindings that are disabled, so disabled bindings can have _any_ collection name. It's a little awkward, but it seems preferable to the alternative of having a separate data structure containing the mappings from resources to collections.

Introduces the `disable` boolean field on derivation transforms, which allows disabling individual transforms of a derivation. The main motivation for this is, given the `disable` fields on capture and materialization bindings, users may expect to be able to also disable derivation transforms in the same way. And the ability to selectively disable transforms is probably pretty convenient in some situations. The implementation is essentially the same as for capture/materialization bindings. Disabled transforms don't get validated, and aren't part of the built specs.

crates/agent/src/publications/specs.rs

crates/models/src/materializations.rs

jgraettinger · 2023-07-18T16:32:01Z

crates/validation/src/materialization.rs

@@ -327,6 +327,7 @@ fn walk_materialization_binding<'a>(
                exclude: fields_exclude,
                recommended: _,
            },
+        ..


nit: use explicit disable: _ as you do w/ Transform ?

jgraettinger · 2023-07-18T16:32:38Z

crates/validation/src/capture.rs

@@ -252,16 +255,16 @@ fn walk_capture_binding<'a>(
    built_collections: &'a [tables::BuiltCollection],
    errors: &mut tables::Errors,
 ) -> Option<capture::request::validate::Binding> {
-    let models::CaptureBinding { resource, target } = binding;
+    let models::CaptureBinding {
+        resource, target, ..


nit: use explicit disable: _ as you do w/ Transform ?

jgraettinger

LGTM % comments

Fixes a bug in the agen publications handler, where it was setting `reads_from` and `writes_to` for derivation and materialization bindings that are disabled. This updates the logic to ignore disabled bindings when setting those values. Also removes the `MaterializationBinding::non_null_resource` function, which was vestigial and confusing, and cleans up some of the `validation` code to no longer rely on that function.

psFried requested a review from jgraettinger July 7, 2023 03:40

psFried force-pushed the phil/disabled-bindings-redux branch from 6066006 to 2fae628 Compare July 11, 2023 21:09

psFried force-pushed the phil/disabled-bindings-redux branch from 2fae628 to 18ca42b Compare July 12, 2023 13:47

psFried force-pushed the phil/disabled-bindings-redux branch from 18ca42b to 6d4726b Compare July 12, 2023 14:18

jgraettinger reviewed Jul 18, 2023

View reviewed changes

jgraettinger approved these changes Jul 18, 2023

View reviewed changes

psFried force-pushed the phil/disabled-bindings-redux branch from 32355cf to 6aa6f76 Compare July 19, 2023 15:11

psFried merged commit 4d99bcf into master Jul 19, 2023
3 checks passed

psFried deleted the phil/disabled-bindings-redux branch July 19, 2023 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

models: disable bindings using a boolean #1100

models: disable bindings using a boolean #1100

psFried commented Jul 7, 2023 •

edited by jgraettinger

Loading

jgraettinger Jul 18, 2023

jgraettinger Jul 18, 2023

jgraettinger left a comment

models: disable bindings using a boolean #1100

models: disable bindings using a boolean #1100

Conversation

psFried commented Jul 7, 2023 • edited by jgraettinger Loading

jgraettinger Jul 18, 2023

Choose a reason for hiding this comment

jgraettinger Jul 18, 2023

Choose a reason for hiding this comment

jgraettinger left a comment

Choose a reason for hiding this comment

psFried commented Jul 7, 2023 •

edited by jgraettinger

Loading