refactor: move metrics, resource_util out of common #15647
Conversation
| pub use rw_iter_util as iter_util; | ||
| pub mod memcmp_encoding; | ||
| pub mod meta_addr; | ||
| pub mod panic; | ||
| pub mod pretty_bytes; | ||
| pub mod prost; | ||
| pub mod query_log; | ||
| pub mod resource_util; | ||
| pub use rw_resource_util as resource_util; |
There was a problem hiding this comment.
Why not changing all the usage? IMO later if we have two ways to import the same module, things can be messy
There was a problem hiding this comment.
Why not changing all the usage?
Because it's widely used in many places. This is kind of "prelude" style. And I now think it's both OK whether or not to change it (but I'm lazy).
IMO later if we have two ways to import the same module, things can be messy
I don't think this may cause any trouble. 👀
| bytes = "1" | ||
| clap = { version = "4", features = ["derive"] } | ||
| easy-ext = "1" | ||
| futures = { version = "0.3", default-features = false, features = ["alloc"] } | ||
| http = "0.2" | ||
| hyper = { version = "0.14", features = ["client"] } | ||
| hytra = { workspace = true } | ||
| itertools = "0.12" | ||
| parking_lot = "0.12" | ||
| pin-project-lite = "0.2" | ||
| prometheus = { version = "0.13" } | ||
| rw_iter_util = { workspace = true } | ||
| rw_resource_util = { workspace = true } | ||
| serde = { version = "1", features = ["derive"] } | ||
| thiserror-ext = { workspace = true } | ||
| tokio = { version = "0.2", package = "madsim-tokio" } | ||
| tonic = { workspace = true } | ||
| tracing = "0.1" | ||
| tracing-subscriber = "0.3.17" |
There was a problem hiding this comment.
Is there any principle for us to decide whether a dependency should be set { workspace = true }?
There was a problem hiding this comment.
For this PR, it's just copied from common/Cargo.toml.
For the general question, I think it won't do any harm to put one in workspace dependency. For widely used dependencies among our crates, it's preferred to put there, so we can manage version in one place.
src/common/metrics/src/lib.rs
Outdated
There was a problem hiding this comment.
Should we delete src/common/src/metrics.rs?
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Because their implementation is self-contained (so it's possible to move out, unlike
arraystuff). To avoidcommon's code being bloated.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.