GH-598: fixes driven from debugging - review 2#743
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on November 14. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
utkarshg6
left a comment
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/sub_lib/accountant.rs:136
| pub payload_size: usize, | ||
| pub service_rate: u64, | ||
| pub byte_rate: u64, | ||
| pub trait AccountableServiceWithTraceLog { |
There was a problem hiding this comment.
This would be additional complexity while searching for logs in the production code. I personally prefer a direct print of log using macros. Introducing a layer of abstraction here apparently can make it harder to search for logs.
| fn trace_log_msg(&self, msg_id: u32) -> String; | ||
| } | ||
|
|
||
| pub trait MessageWithServicesProvided { |
There was a problem hiding this comment.
Prefer enum based approach here
#[derive(Clone, PartialEq, Eq, Debug, Message)]
pub struct ReportServiceProvidedMessage {
pub service: ServiceProvided,
pub service_type: ServiceType,
}
#[derive(Clone, PartialEq, Eq, Debug)]
pub enum ServiceType {
Exit,
Routing,
}
impl ReportServiceProvidedMessage {
pub fn new_exit_service(service: ServiceProvided) -> Self {
Self {
service,
service_type: ServiceType::Exit,
}
}
pub fn new_routing_service(service: ServiceProvided) -> Self {
Self {
service,
service_type: ServiceType::Routing,
}
}
}
impl ReportServiceProvidedMessage {
fn services_provided(&self) -> &ServiceProvided {
&self.service
}
fn service_type(&self) -> ServiceType {
self.service_type
}
}
| pub routing: RoutingServicesConsumed, | ||
| } | ||
|
|
||
| impl AccountableServiceWithTraceLog for ExitServiceConsumed { |
There was a problem hiding this comment.
Lines 219...234 could be replaced by a single log message which just prints the service type similar to other values in the trace log message.
| pub byte_rate: u64, | ||
| } | ||
|
|
||
| pub enum ServiceProvidedTraceLogWrapper<'a> { |
There was a problem hiding this comment.
Again it should be done other way around. You should use the enum ServiceType here. Refer to this comment - https://github.com/MASQ-Project/Node/pull/743/files#r2533460766
| let exit_service_msg = ReportExitServiceProvidedMessage { | ||
| service: service.clone(), | ||
| }; | ||
| let routing_service_msg = ReportRoutingServiceProvidedMessage { service }; |
There was a problem hiding this comment.
This test will change slightly. You should consider a test with 2 cases along with a helper function.
utkarshg6
left a comment
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/logging_utils/accounting_msgs_debug.rs
utkarshg6
left a comment
utkarshg6
left a comment
There was a problem hiding this comment.
Continue at node/src/accountant/mod.rs
| } | ||
| fn handle_report_service_provided_message<AccountingMessage>(&mut self, msg: AccountingMessage) | ||
| where | ||
| AccountingMessage: MessageWithServicesProvided, |
There was a problem hiding this comment.
This should go away once you decide to use the structure instead of the trait implementation. The enum based approach will make things easier for you here. You can thank me later.
| msg.payload_size, | ||
| &msg.paying_wallet, | ||
| ); | ||
| trace_log_wrapper.log_trace(self.logging_kit(MsgIdRequested::New)); |
There was a problem hiding this comment.
This code is worrisome. I think if we had to find the log once we have the log file with us, then at that time, it'll be difficult for us to find this.
This layer of abstraction is the issue, if it wasn't for a use case like logging, I'd appreciate it and here, I'm concerned.
2 participants
No description provided.