feat: Use version to synchronize catalog #749

HuaHuaY · 2022-03-08T07:10:23Z

What's changed and what's your intention?

Use version to synchronize catalog.

There is a version in return value of Create and Drop RPC. Meta will send a notification with a bigger version. create/drop in CatalogConnector will wait until the value in message from ObserverManager is bigger than the return value of Create and Drop RPC.

Checklist

I have written necessary docs and comments
I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

related #567 #361

codecov · 2022-03-08T07:27:09Z

Codecov Report

Merging #749 (11ef557) into main (4e9e18b) will decrease coverage by 0.00%.
The diff coverage is 81.98%.

@@             Coverage Diff              @@
##               main     #749      +/-   ##
============================================
- Coverage     72.42%   72.42%   -0.01%     
  Complexity     2762     2762              
============================================
  Files           918      918              
  Lines         53289    53304      +15     
  Branches       1787     1787              
============================================
+ Hits          38594    38604      +10     
- Misses        13801    13806       +5     
  Partials        894      894

Flag	Coverage Δ
java	`61.17% <ø> (ø)`
rust	`77.14% <81.98%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
rust/common/src/catalog/mod.rs	`66.66% <ø> (ø)`
rust/meta/src/hummock/model/current_version_id.rs	`100.00% <ø> (ø)`
rust/meta/src/manager/notification.rs	`83.33% <ø> (ø)`
rust/meta/src/rpc/service/catalog_service.rs	`100.00% <ø> (ø)`
rust/meta/src/manager/catalog.rs	`75.54% <73.33%> (-0.09%)`	⬇️
rust/frontend/src/observer/observer_manager.rs	`86.66% <75.00%> (+1.48%)`	⬆️
rust/meta/src/model/catalog.rs	`91.22% <86.66%> (-1.63%)`	⬇️
rust/frontend/src/catalog/catalog_service.rs	`86.22% <86.95%> (-0.60%)`	⬇️
rust/frontend/src/scheduler/schedule.rs	`89.84% <100.00%> (ø)`
rust/frontend/src/session.rs	`98.46% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e9e18b...11ef557. Read the comment docs.

BugenZhao

Generally LGTM.

BugenZhao · 2022-03-08T08:04:25Z

rust/frontend/src/catalog/catalog_service.rs

+        while *rx.borrow() < version {
            rx.changed()
                .await
                .map_err(|e| RwError::from(InternalError(e.to_string())))?;


We may extract this to a separated function like wait_version.

+1. The frontend should keep a max committed version in memory and could be initialized when start and fetch from meta. So the meta should stores a global catalog max committed version. This will be helpful for catalog MVCC implementation.

Done. Add a struct CatalogVersionGenerator in CatalogManagerCore of meta.

BugenZhao · 2022-03-08T08:06:54Z

rust/meta/src/manager/catalog.rs

@@ -150,7 +160,7 @@ where
            table.insert(&*self.meta_store_ref).await?;
            core.add_table(table.clone());

-            if let TableInfo::MaterializedView(mview_info) = table.get_info().unwrap() {
+            if let TableInfo::MaterializedView(mview_info) = table.get_info()? {


The info must exist and we should unwrap here.

yezizp2012

Generally LGTM

BowenXiao1999 · 2022-03-09T05:57:40Z

So this PR is saying: Update local catalog cache iff the version from meta is larger than local?

HuaHuaY · 2022-03-09T06:04:34Z

So this PR is saying: Update local catalog cache iff the version from meta is larger than local?

Yes. When CatalogConnector calls create/drop, it will get a new catalog version, suppose "A". create/drop will wait until the version in notification is bigger than "A" or equal to "A". So when create/drop returns, we make sure catalog has been updated.

BowenXiao1999

At least better than check database by name.

Not dive into detail. LGTM.

skyzh · 2022-03-09T06:38:34Z

rust/frontend/src/catalog/catalog_service.rs

-            .get_schema(db_name, schema_name)
-            .is_none()
-        {
+        while *rx.borrow() < version {
            rx.changed()


Is it possible that the version gets changed between *rx.borrow() < version and rx.changed(), and the program gets stuck?

Seems no problem. My fault.

skyzh · 2022-03-09T06:39:55Z

rust/meta/src/manager/notification.rs

@@ -90,7 +90,7 @@ impl NotificationManager {
        }))
        .await
        .into_iter()
-        .collect::<RwResult<()>>()
+        .collect::<Result<()>>()


try_collect?

skyzh · 2022-03-09T06:44:08Z

rust/frontend/src/catalog/catalog_service.rs

-            .get_database(db_name)
-            .is_some()
-        {
+        while *rx.borrow() < version {


It seems that we are using this in a wrong way...

This method does not mark the returned value as seen, so future calls to changed may return immediately even if you have already seen the value with a call to borrow.

should call borrow_and_update instead?

Otherwise .changed will always return immediately, and we are spinning on this thread.

If the version is not latest, the current approach may cause the condition being checked twice. e.g., rx.borrow() returns 1, but as we didn't call changed, the following changed will immediately return, and we will get rx.borrow() == 1 again. But this time, 1 is marked seen, so there won't be problem, and we will wait for the next update.

I'd recommend using borrow_and_update for this scenario, but there won't be problem if we don't modify this.

If the version is not latest, the current approach may cause the condition being checked twice. e.g., rx.borrow() returns 1, but as we didn't call changed, the following changed will immediately return, and we will get rx.borrow() == 1 again. But this time, 1 is marked seen, so there won't be problem.

I'd recommend using borrow_and_update for this scenario, but there won't be problem if we don't modify this.

Yes, you are right. I will change it.

add catalog_version

1fbff48

HuaHuaY requested review from yezizp2012, fuyufjh and skyzh March 8, 2022 07:11

github-actions bot added the type/feature label Mar 8, 2022

yezizp2012 requested a review from BugenZhao March 8, 2022 07:33

BugenZhao reviewed Mar 8, 2022

View reviewed changes

yezizp2012 reviewed Mar 8, 2022

View reviewed changes

BowenXiao1999 approved these changes Mar 9, 2022

View reviewed changes

skyzh reviewed Mar 9, 2022

View reviewed changes

add CatalogVersionGenerator

7e864ba

skyzh approved these changes Mar 10, 2022

View reviewed changes

yezizp2012 approved these changes Mar 10, 2022

View reviewed changes

HuaHuaY added 2 commits March 10, 2022 11:54

Merge branch 'main' into zehua/catalog_waiting_mechanism

50ae2a5

cargo fmt

11ef557

HuaHuaY merged commit fefb7a2 into main Mar 10, 2022

HuaHuaY deleted the zehua/catalog_waiting_mechanism branch March 10, 2022 05:06

HuaHuaY mentioned this pull request Mar 10, 2022

fix: init catalog version from meta store #827

Merged

2 tasks

BowenXiao1999 mentioned this pull request Mar 16, 2022

MVCC of frontend schema #957

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use version to synchronize catalog #749

feat: Use version to synchronize catalog #749

HuaHuaY commented Mar 8, 2022 •

edited

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

BugenZhao left a comment

BugenZhao Mar 8, 2022

yezizp2012 Mar 8, 2022

HuaHuaY Mar 10, 2022

BugenZhao Mar 8, 2022

yezizp2012 left a comment

BowenXiao1999 commented Mar 9, 2022

HuaHuaY commented Mar 9, 2022 •

edited

Loading

BowenXiao1999 left a comment

skyzh Mar 9, 2022

skyzh Mar 9, 2022

skyzh Mar 9, 2022

skyzh Mar 9, 2022 •

edited

Loading

skyzh Mar 9, 2022

skyzh Mar 9, 2022 •

edited

Loading

HuaHuaY Mar 9, 2022

feat: Use version to synchronize catalog #749

feat: Use version to synchronize catalog #749

Conversation

HuaHuaY commented Mar 8, 2022 • edited Loading

What's changed and what's your intention?

Checklist

Refer to a related PR or issue link (optional)

codecov bot commented Mar 8, 2022 • edited Loading

Codecov Report

BugenZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yezizp2012 left a comment

Choose a reason for hiding this comment

BowenXiao1999 commented Mar 9, 2022

HuaHuaY commented Mar 9, 2022 • edited Loading

BowenXiao1999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyzh Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyzh Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuaHuaY commented Mar 8, 2022 •

edited

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

HuaHuaY commented Mar 9, 2022 •

edited

Loading

skyzh Mar 9, 2022 •

edited

Loading

skyzh Mar 9, 2022 •

edited

Loading