-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(kad): enable putting Record
without publisher
#6176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Record
without publisher
Record
without publisher
Record
without publisher
Record
without publisher
Record
without publisher
Record
without publisher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I appreciate the general clean-up of the code, but it does make it hard to review. Given that I currently have limited time for reviews in general, I'd prefer it you could revert unrelated changes unless you feel strongly about them.
pub fn new<K>(key: K, value: Vec<u8>, publisher: Option<PeerId>) -> Self | ||
where | ||
K: Into<Key>, | ||
{ | ||
Record { | ||
key: key.into(), | ||
value, | ||
publisher: None, | ||
publisher, | ||
expires: None, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we don't break the API and instead add a new method Record::new_anonymous
or similar. And then just set a dummy Id here or sth like that, since the actual ID is replaced in Behavior::put_record
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you want. I'd say ("for the record", the pun unintended) I really disagree with the direction, but I don't think my (obvious?) reasons are interesting to anybody now; and it's kinda problems of those who will disentangle the logic the days after. So I can go with it to get anonymous Record
upstream. 👿
On the second thought: if instead of a dummy, separate the (new) logic of put_record
between new
and new_anonymous
the change will be not degrading and even more minimal (add new_anonymous
and remove the line from put_record
).
// single request-response style exchange with a single remote peer. A query | ||
// is made of multiple requests across multiple remote peers. | ||
InboundRequest { request: InboundRequest }, | ||
InboundRequest(InboundRequest), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change and the one below. We should avoid breaking the API unless it is necessary.
On Thu, Oct 9 2025 at 11:59:49 AM -0700, Elena Frank ***@***.***> wrote:
I appreciate the general clean-up of the code, but it does make it
hard to review. Given that I currently have limited time for reviews
in general, I'd prefer it you could revert unrelated changes unless
you feel strongly about them.
That's understandable! No strong feeling for those changes: just that
clean-up. Let me minimize the impact; I guess I'll do it tomorrow, and
will close this one in spite of that minimized -- just in case someone
will start to help you with the reviews and the fresh enthusiasm could
reach this far. X)
|
…ing way. Sorry for that, see <libp2p#6176 (comment)> for some details.
Description
The change is about allowing putting
Record
withpublisher: None
sacrificing republishing.Sorry for so many lateral changes: I had to delve deeper to understand that the change would actually work and during active reading was eliminating redundant pieces which confuse comprehension with misleading supposition (looking for further use of a variable when it is actually just a parameter/argument was most common impediment).
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
withpublisher: Some
); but I need a hint if that's possible to do better due to the same fact of how silent replication is.Change checklist