Improve notifications - #77
Conversation
* Don't wait for sending out notifications in update_node() and map_target() as server nodes only have a one second timeout before they consider registration failed. In some systems, apparently dumping the udp messages takes longer than that. * Improve send_notifications() to only do one db query, not multiple, and call broadcast_datagram() only once. * Improve notification trace logging
89ffbcd to
cd4f610
Compare
philippfalk
left a comment
philippfalk
left a comment
There was a problem hiding this comment.
Looks good for the most part. I had some trouble understanding the new tracker mechanism which was partly caused by a misleading comment.
| /// Creates a peak concurrency tracker object including the required atomics. Each invocation gets | ||
| /// its own tracker / atomics. |
There was a problem hiding this comment.
suggestion: I think this comment is slightly misleading. "Each invocation" implies that every time this is encountered in the code new atomics are created. That's not true because the atomics are declared static (which makes this work) and because this is a macro, "expansion" would actually be more accurate than "invocation".
| /// Creates a peak concurrency tracker object including the required atomics. Each invocation gets | |
| /// its own tracker / atomics. | |
| /// Creates a peak concurrency tracker object including the required atomics. Each expansion site | |
| /// gets its own tracker / atomics. |
| // to catch a potential bottleneck here on big systems. | ||
| let concurrency_tracker = peak_concurrency_tracker!(); | ||
| if let Some(peak) = concurrency_tracker.peaked() { | ||
| log::info!("Concurrent notifications peaked at {peak}",); |
There was a problem hiding this comment.
question: Is info the correct log level here? At least initially, this will be logged quite a number of times as the counter increments. I believe the default log level is warn, so this will not log by default, but this could also be considered debug information.
| MetaRoot::Unknown => 0, | ||
| MetaRoot::Normal(node_id, _) => node_id, | ||
| MetaRoot::Mirrored(group_id) => group_id.into(), | ||
| // Don't wait for notifications to go out because server nodes only waits for a short time for |
There was a problem hiding this comment.
| // Don't wait for notifications to go out because server nodes only waits for a short time for | |
| // Don't wait for notifications to go out because server nodes only wait for a short time for |
2 participants
See commits.
Fixes https://github.com/ThinkParQ/beegfs-core/issues/4772