Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coord: Make storage usage interval configurable #14787

Merged
merged 6 commits into from
Sep 13, 2022

Conversation

jkosh44
Copy link
Contributor

@jkosh44 jkosh44 commented Sep 12, 2022

This commit makes the storage usage collection interval configurable, so that it is easier to test.

Works towards resolving #3739

Motivation

This PR adds a known-desirable feature.

Checklist

This commit makes the storage usage collection interval configurable,
so that it is easier to test.

Works towards resolving MaterializeInc#3739
@jkosh44 jkosh44 force-pushed the storage-usage-table branch from 974a109 to c5c1cf6 Compare September 12, 2022 22:23
Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

LGTM modulo nit about command line argument naming.

env = "STORAGE_USAGE_COLLECTION_INTERVAL_SEC",
default_value = "3600"
)]
storage_usage_collection_interval_sec: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a storage_usage_collection_interval: Duration directly and tell clap to parse it using the parse_duration crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have our own parse_duration function which I think we've used in the past to parse command line arguments. Should we use that instead? Or does the crate do a better job?

@jkosh44 jkosh44 enabled auto-merge (squash) September 13, 2022 20:09
@jkosh44
Copy link
Contributor Author

jkosh44 commented Sep 13, 2022

@benesch The parse_duration crate is no longer being update https://github.com/zeta12ti/parse_duration

IMPORTANT: This repository is no longer being updated. Before deciding to use it, check if any of the issues are deal breakers. In particular, this crate should not be used with untrusted input (see zeta12ti/parse_duration#21).

The crate also uses an old version of the num crate. They use version 0.2.0, but the most recent version is 0.4.0.
https://github.com/zeta12ti/parse_duration/blob/089e7988b0677d31c45f5a72216abf9c5155600f/Cargo.toml#L20
https://crates.io/crates/num

This causes us to have duplicate dependencies on the num crate. We have an explicit policy not to allow duplicate dependencies, and to either update the crate upstream or fork it.

materialize/deny.toml

Lines 3 to 8 in 0357d26

# Do not add exemptions for duplicate dependencies! Duplicate dependencies slow
# down compilation, bloat the binary, and tickle race conditions in `cargo doc`
# (see rust-lang/cargo#3613). Submit PRs upstream to remove duplicated
# transitive dependencies. If necessary, use patch directives in the root
# Cargo.toml to point at a Materialize-maintained fork that avoids the
# duplicated transitive dependencies.

Considering that the crate is dead, do we want to fork it or go a different route?

@jkosh44 jkosh44 merged commit 149a59c into MaterializeInc:main Sep 13, 2022
@jkosh44 jkosh44 deleted the storage-usage-table branch September 13, 2022 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants