feat(kad): enable putting Record without publisher #6177
Conversation
…ing way. Sorry for that, see <libp2p#6176 (comment)> for some details.
skaunov
changed the title
Record with None publisher in a confus…Record with None publisher in a confus…
jxs
changed the title
Record with None publisher in a confus…
| if record.publisher.is_some() { | ||
| record.publisher = Some(*self.kbuckets.local_key().preimage()) | ||
| } |
There was a problem hiding this comment.
nit:
| if record.publisher.is_some() { | |
| record.publisher = Some(*self.kbuckets.local_key().preimage()) | |
| } | |
| record.publisher = record.publisher.map(|| Some(*self.kbuckets.local_key().preimage())) |
There was a problem hiding this comment.
I'm absolutely with you on an improving, so could you explain to me some benefits of this nit? I mean to my eye the former is better because map which fully disregards the thing it "maps" is confusing and the former version better convey semantics of what goings on: it doesn't matter what was in the Option - it will be changed to the value from the structure.
There was a problem hiding this comment.
I mean if the aim is to have one less allocation would keeping if condition and .replace inside of it help? Instead of assigning the new value... Actually I have this question for some time already: how much better such approach or it's just optimized away. But savings are so insignificant in these cases I had no chance to ask yet.
| // this test relies on counting republished `Record` against `records.len()` | ||
| records.retain(|r| r.publisher.is_some()); |
There was a problem hiding this comment.
can we then add a new test for new_anonymous?
There was a problem hiding this comment.
I have not get any new ideas since #6176 (comment).
Notes & open questions
I don't have a fine test for this in mind as it doesn't really do anything (because replication context doesn't yield an event). Simultaneously I'm not really satisfied with the approach I took to adapt/fix the test (retaining only Record with publisher: Some); but I need a hint if that's possible to do better due to the same fact of how silent replication is.
Co-authored-by: João Oliveira <hello@jxs.pt>
2 participants
Description
superseeds #6176, see #6176 (comment) for some details.