Collator protocol followup #1741
Conversation
montekki
added
A0-please_review
| /// If `id` is `None` this is a validator side of the protocol. | ||
| pub fn new(id: Option<CollatorId>) -> Self { | ||
| /// Caller must provide a registry for prometheus metrics. | ||
| pub fn new(id: Option<CollatorId>, registry: &prometheus::Registry) |
There was a problem hiding this comment.
I think &Registry is not always available, real_overseer accepts Option<&Registry>. we could do the same and call register function instead of try_register.
Wouldn't it be better to make ProtocolSide public and pass it here instead?
ProtocolSide could have two constructors:
impl ProtocolSide {
pub fn validator(registry: Option<&Registry>) -> Result<Self> {
...
}
pub fn collator(id: CollatorId, registry: Option<&Registry>) -> Result<Self> {
...
}
}There was a problem hiding this comment.
In the original PR there were grumbles agains it
There was a problem hiding this comment.
But how was it resolved? We still are making a choice at startup depending on whether id is None or not?
There was a problem hiding this comment.
Yep, I see no other options
ordian
left a comment
ordian
left a comment
There was a problem hiding this comment.
Looks good, just a couple of questions.
Co-authored-by: Andronik Ordian <[email protected]>
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
I think that the recently_removed_heads thing is nice, though it necessarily limits us to precisely one update-duration of leeway for late blocks. As has already been discussed, we're implicitly assuming that we get precisely one of these updates per block, and I'm not certain that's a formal requirement for the overseer.
That said, I don't see a better way of dealing with this until we change the ActiveLeavesUpdate to explicitly name parent-child relationships, which would allow us to specify a particular duration to keep the leaves.
| { | ||
| type Metrics = Metrics; | ||
| // The actual `Metrics` type depends on whether we're on the collator or validator side. | ||
| type Metrics = (); |
There was a problem hiding this comment.
I don't really understand why we expose the Metrics type in the trait if it's ok to just fake it and use a different type internally. I assume this is fine as @ordian has already approved this PR, but it may be worth considering whether we could remove type Metrics from trait Subsystem.
If on the other hand there's a reason to publish this type, then we really should be distributing it via an enum.
|
bot merge |
|
Trying merge. |
ghost
merged commit * master: Collator protocol followup (#1741) Revert "Remove Old Service, 2nd try (#1732)" (#1758) Remove Old Service, 2nd try (#1732) collation-generation: guide and tidying (#1753) Companion for #7111 (Introduce `cancel_proposal` and `blacklist`) (#1728) Parachains: Introduce a dummy module to include the Origin. (#1749) provisioner tests: remove tokio from dev-dependencies (#1745)
3 participants
Fixes #1694