adapter: handle AddSink (storage exports) via catalog implications #34506
Conversation
teskje
left a comment
teskje
left a comment
There was a problem hiding this comment.
LGTM, if you can clarify the "we don't support sink alterations" comment.
| // Sink alterations are not yet supported - they would require | ||
| // re-creating the storage export with new parameters. |
There was a problem hiding this comment.
Hm, but we do support ALTER SINK ... SET FROM. Am I missing something?
There was a problem hiding this comment.
We don't support them in the implications framework. But I can add that as a commit on top
There was a problem hiding this comment.
I'd be happy with modifying the comment to make it clear that "not yet supported" is in the context of the implications, not in the context of Materialize!
There was a problem hiding this comment.
I'll just restore the old debug logging 😅
| async fn handle_create_sink( | ||
| &mut self, | ||
| storage_policies_to_initialize: &mut BTreeMap<CompactionWindow, BTreeSet<GlobalId>>, | ||
| _item_id: CatalogItemId, |
There was a problem hiding this comment.
Why pass this parameter if we don't need it?
| // pulling the rug from under us! | ||
|
|
||
| // TODO: Maybe in the future, pass those holds on to storage, to hold on | ||
| // │ to them and downgrade when possible? |
There was a problem hiding this comment.
| // │ to them and downgrade when possible? | |
| // to them and downgrade when possible? |
| return; | ||
| } | ||
| Err(e) => { | ||
| ctx.retire(Err(e)); |
There was a problem hiding this comment.
Nit: If we need to call ctx.retire in every branch, we could pull it after the match, to make sure we never forget to do it. Also to remove a tiny amount of duplication.
There was a problem hiding this comment.
I'll remove it, but I'm tempted to put a wrapper around the result massaging, to make sure we can't early-return without retiring the context.
aljoscha
force-pushed
the
adapter-implications-create-sink
branch
from
d0d5f69 to
8e60b80
Compare
2 participants
No description provided.