estuary / ui

A web based UI to assist in working with Estuary Flow
https://dashboard.estuary.dev/
Other
12 stars 1 forks source link

Cannot exclude "flow_document" for delta updates materialization #840

Closed dyaffe closed 9 months ago

dyaffe commented 11 months ago

Bug The UI prevents one from excluding the field flow_document for delta updates materializations. That field is only required for standard materializations and is strictly necessary to exclude sometimes.

travjenkins commented 11 months ago

@dyaffe - As far as I can tell we have no code that prevents this. I tested a few scenarios and that field is always marked as required. What scenario are you using to have this field not be required?

dyaffe commented 11 months ago

It is never required when one does delta updates. We actively need to exclude it whenever a collection has fields that are too long to materialize to a destination.

travjenkins commented 11 months ago

@psFried / @jgraettinger - I feel like this is something we should probably add to the backend. The validated columns populated in the draft_spec table could mark flow_document as recommended and not required if the delta updates option is enabled.

Thoughts? I am fine hardcoding to allow flow_document as being excluded whenever... just trying to think of the best way to make this future proof and reduce the chance of weird UX where a user can cause themselves issues.

travjenkins commented 11 months ago

Moving to connectors as we'll want the backend to mark flow_document as optional when the delta updates are done.

williamhbaker commented 10 months ago

I think the connector is already doing the right thing here: If it gets a Validate request with a binding set to delta updates, it will only recommend the flow_document field rather than requiring it: https://github.com/estuary/connectors/blob/c3402c7ce843c6ffae82194d4ad7052ce7eabf64/materialize-sql/type_mapping.go#L494-L499

Here's something I am observing when testing on a local stack that might explain the original bug description:

  1. Create a materialization using standard updates to a SQL materialization, like materialize-postgres. Publish it using standard updates.
  2. Initiate an edit of the materialization. Click "See Fields" while it is still set to standard updates. Observe that the flow_document field is required and can't be excluded. This is expected because it is still using standard updates.
  3. Click the "delta updates" checkbox, then click "next". The field selection list is still shown, and the flow_document field is still indicated as required. The clicking of "next" completes almost instantly when it does this, which suggests to me that the connector is not being brought into the loop to do a Validate on the updated resource configuration.
  4. If you click "see fields" again at this point, the connector will get a Validate call and the field selection list is updated accordingly, with the flow_document field now marked as recommended instead of required, which is what we'd expect the connector to say when doing a Validate on a binding with delta updates.

I'm not sure if that's the exact sequence of events @dyaffe was seeing, but it does seem like a potential source of confusion. Maybe editing the resource config should clear out a previous list of available fields, which would require clicking "see fields" again?

dyaffe commented 10 months ago

Those are exactly the steps I took @williamhbaker and that suggestion would definitely help.

travjenkins commented 9 months ago

Thanks @williamhbaker ! I am moving this back into the UI as we need to update our calls to fix this.

travjenkins commented 9 months ago

We now have a warning in the UI that tells them they may need to refresh after a setting has changed.