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

Introduce janus_main #176

Merged
merged 1 commit into from
May 19, 2022
Merged

Introduce janus_main #176

merged 1 commit into from
May 19, 2022

Conversation

tgeoghegan
Copy link
Contributor

Factors out common binary startup work like parsing options and config,
setting up metrics and logging and setting up a datastore connection
into a function binary_utils::janus_main.

@tgeoghegan tgeoghegan requested a review from a team as a code owner May 18, 2022 23:32
@tgeoghegan
Copy link
Contributor Author

For #105, I am going to need to add collect_job_creator and collect_job_driver targets, so this will be useful for that. Next up, I'm going to factor out the task enumeration logic in aggregation_job_creator into something that can be re-used in collect_job_creator.

Comment on lines 14 to 20
#[structopt(
name = "janus-aggregator",
about = "PPM aggregator server",
rename_all = "kebab-case",
version = env!("CARGO_PKG_VERSION"),
)]
struct Options {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This struct (and the equivalent ones in aggregation_job_driver.rs and aggregation_job_creator.rs exist solely so we can provide a #[structopt(...)] block with the name and about values.

fn common_options(&self) -> &CommonBinaryOptions;
}

// Common options that are used by all Janus binaries.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is deliberately not a doccomment to work around a known issue in structopt. It's not clear if/when that will be fixed, but in any case, I learned today that structopt has been deprecated in favor of functionality built into clap.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?

Suggested change
// Common options that are used by all Janus binaries.
#[cfg_attr(doc, doc = "Common options that are used by all Janus binaries.")]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works, thanks!

let _metrics_exporter = install_metrics_exporter(config.metrics_config())
.context("failed to install metrics exporter")?;

info!(?common_options, ?config, "Starting aggregation job creator");
Copy link
Member

Choose a reason for hiding this comment

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

nit: this info line refers to the aggregation job creator specifically; I suspect we can get away without referencing which job is starting up, so the easiest fix might be to just change the string to something like "Starting up".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think in practice it'll be obvious which binary logged this because the log event will be annotated with a Kubernetes pod or deployment.

fn common_options(&self) -> &CommonBinaryOptions;
}

// Common options that are used by all Janus binaries.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?

Suggested change
// Common options that are used by all Janus binaries.
#[cfg_attr(doc, doc = "Common options that are used by all Janus binaries.")]

Factors out common binary startup work like parsing options and config,
setting up metrics and logging and setting up a datastore connection
into a function `binary_utils::janus_main`.
@tgeoghegan tgeoghegan merged commit 4e909d7 into main May 19, 2022
@tgeoghegan tgeoghegan deleted the timg/janus-main-function branch October 12, 2022 19:22
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.

3 participants