dbt-materialize: remove duplicated readiness SQL and deploy boilerplate - #38072
Conversation
4682492 to
6128ab9
Compare
jubrad
left a comment
jubrad
left a comment
There was a problem hiding this comment.
Nice cleanup, and I checked the risky part: the two readiness queries were identical and are_clusters_ready guarantees an entry for every requested cluster, so the is_cluster_ready delegation is faithful.
Summary
1 consideration
1 nit
1 praise
written by claude on behalf of @jubrad
| @@ -34,13 +34,7 @@ | |||
| #} | |||
|
|
|||
| {% set current_target_name = target.name %} | |||
There was a problem hiding this comment.
nit — current_target_name is now unused in this macro. Its only consumer was the error message that moved into internal_get_deployment_config, unlike deploy_init/deploy_promote/deploy_cleanup, which still log with it.
{% set current_target_name = target.name %} {# dead #}
{% set target_config = internal_get_deployment_config() %}written by claude on behalf of @jubrad
There was a problem hiding this comment.
Good catch, removed. The other three deploy operations still use theirs for logging so those stay.
| error when the `deployment` variable has no entry for it. Every deploy operation | ||
| starts from this configuration. | ||
| #} | ||
| {% macro internal_get_deployment_config() %} |
There was a problem hiding this comment.
consider — internal_get_deployment_config is now shared by all four deploy operations, but it lives in deploy_init.sql, which is where the other internal_* helpers are all deploy_init-only (internal_copy_schema_grants, internal_copy_cluster_grants, and so on). Resolution works either way since dbt macros are package-global, but someone reading deploy_promote.sql has no reason to look here for it.
Would you consider a small deploy_helpers.sql (or dropping it next to deploy_permission_validation.sql, which is already a shared-helpers file)? The doc comment is good and would carry over unchanged.
written by claude on behalf of @jubrad
There was a problem hiding this comment.
Agreed, moved it to a new deploy/deploy_helpers.sql with the doc comment carried over unchanged. Went with a dedicated file rather than deploy_permission_validation.sql since that one is named for what it does.
| {% endif %} | ||
|
|
||
| {{ log("Checking cluster readiness for: " ~ cluster, info=True) }} | ||
| {% set statuses = are_clusters_ready([cluster], lag_threshold) %} |
There was a problem hiding this comment.
praise — I diffed the deleted SQL against are_clusters_ready and the only difference was c.name = 'x' versus c.name IN ('x'), so this really is behaviour-preserving. Worth noting for other reviewers that the cluster_not_found case is preserved too, not by this macro but by the fill-in loop at the bottom of are_clusters_ready that inserts a failing/cluster_not_found entry for every requested cluster missing from the result set. Keeping the {% if execute %} guard is also the right call, since it preserves the parse-time no-return that await_cluster_ready relies on.
written by claude on behalf of @jubrad
There was a problem hiding this comment.
Thanks for double checking the diff! The cluster_not_found fill-in loop was the part I wanted a second pair of eyes on!
6128ab9 to
1843290
Compare
2 participants
is_cluster_readycarried its own copy of the readiness query thatare_clusters_readyalready runs, so the two had to be kept in step by hand, and it now just calls the multi cluster macro with one cluster. This also pulls the repeated deployment config lookup out of the four deploy operations, drops a cache linking method that nothing can reach because the hook that calls it is a no-op here, and removes an argument from the seed helper that dbt never passes.Test plan: no behavior change intended, so this relies on the existing deploy and seed tests.