refactor(meta): improve create mview / scheduler readability

[BugenZhao]

Signed-off-by: Bugen Zhao [email protected]

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

As title. This PR makes create_materialized_view and Scheduler more readable by adding some comments and extracting common snippets.

Refer to a related PR or issue link (optional)

[codecov]

Codecov Report

Merging #3748 (5d85472) into main (8fbe3f9) will increase coverage by 0.02%.
The diff coverage is 55.12%.

@@            Coverage Diff             @@
##             main    #3748      +/-   ##
==========================================
+ Coverage   74.20%   74.22%   +0.02%     
==========================================
  Files         797      797              
  Lines      112204   112191      -13     
==========================================
+ Hits        83256    83275      +19     
+ Misses      28948    28916      -32     

Flag	Coverage Δ
rust	74.22% <55.12%> (+0.02%)	⬆️

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

Impacted Files	Coverage Δ
src/meta/src/lib.rs	1.33% <ø> (ø)
src/meta/src/manager/hash_mapping.rs	98.85% <ø> (+1.43%)	⬆️
src/meta/src/stream/stream_manager.rs	76.73% <49.67%> (+1.54%)	⬆️
src/meta/src/stream/scheduler.rs	91.63% <70.58%> (+2.95%)	⬆️
src/meta/src/stream/meta.rs	54.01% <100.00%> (+0.14%)	⬆️
src/storage/src/hummock/local_version_manager.rs	81.40% <0.00%> (+0.11%)	⬆️
src/common/src/types/ordered_float.rs	24.90% <0.00%> (+0.19%)	⬆️
src/frontend/src/expr/utils.rs	98.99% <0.00%> (+0.25%)	⬆️
... and 3 more

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

[xx01cyx]

We might distinguish different types of mappings in a clearer way. For example, when we say vnode_mapping, what does it refer to? Mapping from vnode to parallel unit, or to actor id, or both? After we make a decision on this, we can do some renamings on parallel_unit_mapping and other variables with alike names.

[xx01cyx]

Suggested change

	let actor_to_vnode_parallel_unit_mapping = {
	let actor_to_vnode_mapping = {

vnode_mapping might be enough, for vnode_mapping stands for the mapping from vnode to parallel unit elsewhere in the code as well (e.g. hash_mapping.rs).

[BugenZhao]

Actually I didn't realize the difference between ParallelUnitMapping and ActorMapping below and had been confused for a while. 🤣
https://github.com/singularity-data/risingwave/blob/a6449facc7a1380aab8ac41b642a1d28724a5b6e/src/meta/src/stream/stream_manager.rs#L404-L416

[xx01cyx]

This is proposed by chi actually. It is designed to set apart

• mapping from vnode to parallel unit
• mapping from vnode to actor

just for clarity.

[xx01cyx]

We might distinguish different types of mappings in a clearer way. For example, when we say vnode_mapping, what does it refer to? Mapping from vnode to parallel unit, or to actor id, or both? After we make a decision on this, we can do some renamings on parallel_unit_mapping and other variables with alike names.

I think this could be deferred.

[BugenZhao]

We might distinguish different types of mappings in a clearer way. For example, when we say vnode_mapping, what does it refer to? Mapping from vnode to parallel unit, or to actor id, or both? After we make a decision on this, we can do some renamings on parallel_unit_mapping and other variables with alike names.

Really agree with this! I'd like to refine it continuously while working on #3750.