Separate the progress logging stream, use dyn trait instead of String for timestamps #353
Conversation
timely/src/logging.rs
Outdated
| pub trait ProgressEventTimestamp: std::fmt::Debug + std::fmt::Display + std::any::Any { | ||
| /// Encodes a typed reference into a binary buffer. |
There was a problem hiding this comment.
Just to check, Timestamp has already Debug+Any+Abomonation/Serialize. Is the motivation for the trait to add Display, or because the communication::Data trait isn't actually helpful enough?
There was a problem hiding this comment.
It’s because Data can’t be turned into a dyn trait, because entomb takes a type parameter. Should probably add a comment to remind future readers.
|
Thanks very much! I think this looks good, though I have to admit not knowing about ease/pain of logging out. Strings look pretty good, but other formats could be hard and I wouldn't know. This does the good things (imo) of extracting the progress logging to a new stream so that it can be turned off cheaply. I had a comment or two about the rationale for some traits, not because I doubt the rationale but more to write it down so that in a few months I can remember why it wasn't another way. :) |
…amic type information instead
utaal
force-pushed
the
log_progress_accurately
branch
from
d3aec0d to
08964d8
Compare
|
I changed things to be serialization-agnostic, but to still offer the option of recovering the real timestamp type in the registered log function. This way, someone who needs detailed, typed information, can downcast to the concrete types of the timestamp present in the dataflow. I also actually sent native types and not formatted strings, which I had failed to do previously (in |
|
|
||
| #[derive(Serialize, Deserialize, Abomonation, Debug, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)] | ||
| /// Encapsulates Any and Debug for dynamically typed timestamps in logs | ||
| pub trait ProgressEventTimestamp: std::fmt::Debug + std::any::Any { |
There was a problem hiding this comment.
Random comment, not a change but: I bet this trait eventually becomes useful beyond progress events. At least, assuming there is any other reason to report information about timestamps (e.g., maybe other timestamp things, I dunno) they'll probably make use of this too. Again: nothing to change, just thinking out loud.
There was a problem hiding this comment.
Yea, I thought the same when I wrote it, but it seemed unnecessary to push it to a separate module at this stage.
| fn type_name(&self) -> &'static str { std::any::type_name::<T>() } | ||
| } | ||
|
|
||
| /// A vector of progress updates in logs |
There was a problem hiding this comment.
Minor, but this would be a great place to explain the need for a trait (vs a type or a struct that matches the same description).
There was a problem hiding this comment.
Added to the doc in the latest commit.
| let mut messages = Box::new(Vec::with_capacity(changes.len())); | ||
| let mut internal = Box::new(Vec::with_capacity(changes.len())); |
There was a problem hiding this comment.
I'm super confused that this works. I would have thought that as soon as this is boxed up you lose access to the push methods. Or is it because there is an auto-conversion from Box<T> to Box<dyn Trait> for any T: Trait? No change needed, just surprising to me!
There was a problem hiding this comment.
I believe push works because of https://doc.rust-lang.org/std/boxed/struct.Box.html#impl-Deref (Deref impl for Box). And yes, you can upcast a Box<T> to a Box<dyn Trait> for the same reason you can upcast a &T to a &dyn Trait for T: Trait.
There was a problem hiding this comment.
To clarify, the type of messages and internal is still the concrete, non-dyn vector type. The upcast happens on assignment to the target struct below.
timely/src/progress/broadcast.rs
Outdated
| messages: Box::new(messages), | ||
| internal: Box::new(internal), |
There was a problem hiding this comment.
Yeah, this is what I thought you would need to do. Is there a reason this is needed here but not previously? If so I think we should doc it, and if not we should unify up the two cases to look the same (maybe just aesthetics, but also to avoid randoms (myself) asking this question in the future).
There was a problem hiding this comment.
The asymmetry was unintentional (and I guess a potential perf defect). Fixed in the last commit.
There was a problem hiding this comment.
I think this looks good. I have some questions, but none of them need to block merging this in to the PR (and potentially then merging the PR). I'll not merge right away in case any of the comments seem actionable to you, but otherwise I'm happy to do it now!
|
I believe I addressed the comments: so go ahead and merge if the latest changes look good! Thanks for the review. |
* Actually log progress updates * remove commented code, improve comments * Separate the progress logging stream, use dyn trait instead of String for timestamps (#353) * Separate the progress logging stream, use dyn trait instead of String for timestamps * Remove the serialization machinery from progress logging, provide dynamic type information instead * Add example for progress logging * Always box logging progress vectors on construction * Explain why we need the `ProgressEventTimestampVec` trait Co-authored-by: Andrea Lattuada <[email protected]>
2 participants
This only supports cfg(not(feature = "bincode")), bincode support upcoming.
Currently based on the
log_progress_accuratelybranch.@frankmcsherry