deployment: add monitoring endpoint port to the deployment file #7957
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Benchmark movements: No major performance changes detected. |
There was a problem hiding this comment.
Reviewable status: 0 of 27 files reviewed, 2 unresolved discussions
crates/apollo_deployments/resources/testing_secrets_locally.json line 1 at r1 (raw file):
{
What is the difference between this file and crates/apollo_deployments/resources/testing_secrets.json ?
crates/apollo_deployments/src/deployments/distributed.rs line 278 at r1 (raw file):
fn get_ports(&self) -> BTreeMap<ServicePort, u16> { let mut ports = BTreeMap::new(); ports.insert(ServicePort::MonitoringEndpoint, MONITORING_ENDPOINT_DEFAULT_PORT);
Please:
- define iteration over
ServicePort(check forEnumIterin the code) - define iteration over the enum variants, and use match case to set the value
The above ensures that once a new enum variant is added there will be a compliation error until it is set in the map.
Code quote:
ports.insert(ServicePort::MonitoringEndpoint, MONITORING_ENDPOINT_DEFAULT_PORT);
There was a problem hiding this comment.
Reviewable status: 0 of 27 files reviewed, 2 unresolved discussions (waiting on @nadin-Starkware)
crates/apollo_deployments/src/deployments/distributed.rs line 278 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please:
- define iteration over
ServicePort(check forEnumIterin the code)- define iteration over the enum variants, and use match case to set the value
The above ensures that once a new enum variant is added there will be a compliation error until it is set in the map.
(same for all 3 functions)
nadin-Starkware
force-pushed
the
07-14-deployment_add_monitoring_endpoint_port_to_the_deployment_file
branch
from
e8a46e2 to
4812829
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 27 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
crates/apollo_deployments/src/deployments/distributed.rs line 278 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
(same for all 3 functions)
Done.
crates/apollo_deployments/resources/testing_secrets_locally.json line 1 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
What is the difference between this file and
crates/apollo_deployments/resources/testing_secrets.json?
This shouldn't be committed, Deleting it
There was a problem hiding this comment.
Reviewable status: 0 of 27 files reviewed, 2 unresolved discussions
crates/apollo_deployments/src/deployments/distributed.rs line 286 at r2 (raw file):
ServicePort::HttpServer => match self { DistributedNodeServiceName::HttpServer => None, _ => None,
OK, I didn't think through my previous suggestion, apologies 😅
My concern here (and with _ branches in general) is that it prevents the compiler from protecting us in case we add another variant: then we'd have to go through all the usages and find the relevant ones.
Is there a way to unify these with the component config fns? Maybe incorporate the enum in there? I'm scared of hiding these knobs deep down in the code, and having to back-trace on how to change them later on.
Code quote:
_ => None,
nadin-Starkware
force-pushed
the
07-14-deployment_add_monitoring_endpoint_port_to_the_deployment_file
branch
3 times, most recently
from
f8c34ad to
e8b07e3
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 28 files reviewed, 1 unresolved discussion
crates/apollo_deployments/src/deployments/consolidated.rs line 24 at r4 (raw file):
}; use crate::service::{GetComponentConfigs, NodeService, ServiceNameInner};
Restore new line please
nadin-Starkware
force-pushed
the
07-14-deployment_add_monitoring_endpoint_port_to_the_deployment_file
branch
from
e8b07e3 to
be56ded
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 28 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)
crates/apollo_deployments/src/deployments/consolidated.rs line 24 at r4 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Restore new line please
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 28 files reviewed, 2 unresolved discussions
crates/apollo_deployments/src/service.rs line 281 at r4 (raw file):
fn get_service_ports(&self) -> BTreeSet<ServicePort>; fn get_ports(&self) -> BTreeMap<ServicePort, u16> {
Could you please rename to something more descriptive in comparsion to get_service_ports?
Maybe
get_ports -> get_service_port_values or get_service_port_mapping?
Code quote:
fn get_ports(
nadin-Starkware
force-pushed
the
07-14-deployment_add_monitoring_endpoint_port_to_the_deployment_file
branch
from
be56ded to
b345a55
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 28 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
crates/apollo_deployments/src/service.rs line 281 at r4 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Could you please rename to something more descriptive in comparsion to
get_service_ports?Maybe
get_ports->get_service_port_valuesorget_service_port_mapping?
Done.
nadin-Starkware
force-pushed
the
07-14-deployment_add_monitoring_endpoint_port_to_the_deployment_file
branch
from
b345a55 to
b69ce7b
Compare
There was a problem hiding this comment.
Reviewed 22 of 27 files at r1, 3 of 5 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: 27 of 28 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 1 of 5 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @nadin-Starkware)
No description provided.