Skip to content

Conversation

@zbzbw
Copy link
Contributor

@zbzbw zbzbw commented Mar 18, 2022

Signed-off-by: Bowen Zhou [email protected]

What's changed and what's your intention?

As title. This PR separates mv creation in catalog manager into two parts:

  1. Set a "in progress" sign and add reference count
  2. If mv has been created successfully, update the catalog as usual.

Noted that we should also refactor create_source according to this.

Checklist

  • I have written necessary docs and comments

Refer to a related PR or issue link (optional)

Resolve #1019.
Maybe we should merge #1065 first

@zbzbw zbzbw requested review from BugenZhao and yezizp2012 March 18, 2022 07:11
@github-actions github-actions bot added the type/feature Type: New feature. label Mar 18, 2022
@zbzbw zbzbw requested a review from st1page March 18, 2022 07:16
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #1066 (cceea5e) into main (e106294) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #1066      +/-   ##
============================================
- Coverage     71.76%   71.69%   -0.08%     
  Complexity     2766     2766              
============================================
  Files           996      996              
  Lines         84016    84100      +84     
  Branches       1790     1790              
============================================
+ Hits          60293    60294       +1     
- Misses        22832    22915      +83     
  Partials        891      891              
Flag Coverage Δ
java 61.01% <ø> (ø)
rust 74.17% <0.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rust/meta/src/manager/catalog_v2.rs 0.00% <0.00%> (ø)
rust/meta/src/rpc/service/catalog_service_v2.rs 0.00% <0.00%> (ø)
rust/common/src/types/ordered_float.rs 22.50% <0.00%> (+0.19%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Ok(version)
}

async fn create_materialized_view_inner(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about change create_mview_in_stream_manager to create_materialized_view_inner, and call create_materialized_view_inner directly in create_materialized_view to make it simple.

Copy link
Member

@yezizp2012 yezizp2012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

@yezizp2012
Copy link
Member

Good job, plz help to handle create_source as well after this PR merged.

Signed-off-by: Bowen Zhou <[email protected]>
@yezizp2012 yezizp2012 marked this pull request as ready for review March 18, 2022 08:56
Signed-off-by: Bowen Zhou <[email protected]>
@zbzbw zbzbw enabled auto-merge (squash) March 18, 2022 09:31
@zbzbw zbzbw disabled auto-merge March 18, 2022 09:38
@zbzbw zbzbw enabled auto-merge (squash) March 18, 2022 09:43
@zbzbw zbzbw merged commit de8651c into main Mar 18, 2022
@zbzbw zbzbw deleted the zbw/create-mview-v2-refactor branch March 18, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature Type: New feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

meta: handling duplicate naming mv create when there's already a mv being creating

3 participants