protocols/kad: Improve options to efficiently retrieve #2712
GetRecordapi
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -452,6 +452,11 @@ where | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Returns the currently configured `replication_factor`. | ||||||
| pub fn replication_factor(&self) -> NonZeroUsize { | ||||||
| self.queries.config().replication_factor | ||||||
| } | ||||||
|
|
||||||
|
||||||
| /// Gets an iterator over immutable references to all running queries. | ||||||
| pub fn iter_queries(&self) -> impl Iterator<Item = QueryRef<'_>> { | ||||||
| self.queries.iter().filter_map(|query| { | ||||||
|
|
@@ -682,37 +687,50 @@ where | |||||
| /// | ||||||
| /// The result of this operation is delivered in a | ||||||
| /// [`KademliaEvent::OutboundQueryCompleted{QueryResult::GetRecord}`]. | ||||||
| pub fn get_record(&mut self, key: record::Key) -> QueryId { | ||||||
| let record = if let Some(record) = self.store.get(&key) { | ||||||
| if record.is_expired(Instant::now()) { | ||||||
| self.store.remove(&key); | ||||||
| None | ||||||
| } else { | ||||||
| Some(PeerRecord { | ||||||
| peer: None, | ||||||
| record: record.into_owned(), | ||||||
| }) | ||||||
| } | ||||||
| } else { | ||||||
| None | ||||||
| }; | ||||||
mxinden marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| let target = kbucket::Key::new(key.clone()); | ||||||
| let info = QueryInfo::GetRecord { | ||||||
| key, | ||||||
| count: 1, | ||||||
| record_to_cache: record.as_ref().map(|r| r.record.clone()), | ||||||
| cache_candidates: BTreeMap::new(), | ||||||
| }; | ||||||
| let peers = self.kbuckets.closest_keys(&target); | ||||||
| let inner = QueryInner::new(info); | ||||||
| let id = self.queries.add_iter_closest(target.clone(), peers, inner); // (*) | ||||||
|
||||||
| let id = self.queries.add_iter_closest(target.clone(), peers, inner); // (*) | |
| let id = self.queries.add_iter_closest(target.clone(), peers, inner); |
Is this reference this relevant?
There was a problem hiding this comment.
removed, not sure what this was from
Outdated
There was a problem hiding this comment.
Say we discover multiple records for the same key. With the above only the first record is replicated. libp2p-kad can not know which of the records is the one to be cached, i.e. it can not make a qualified judgement on which of the records is the best one.
How about removing this automated publishing mechanism and depend on the user to call Kademlia::put_record_to instead?
rust-libp2p/protocols/kad/src/behaviour.rs
Lines 778 to 798 in 6da8497
Either way, the comment on put_record_to needs updating.
There was a problem hiding this comment.
I have removed the caching, but I am confused why the behaviour of put_record_to changed if I only changed the behaviour of get_record, does that rely on that caching to do this?
There was a problem hiding this comment.
According to the Kademlia paper:
For caching purposes, once a lookup succeeds, the requesting node stores the (key, value) pair at the closest node it observed to the key that did not return the value.
Before this pull request, when we did a Kademlia::get_record with a quorum of 1, we could replicate the (key, value) pair to the closest node that did not have the pair. But if we did a quorum of >1, the Kademlia::get_record query would potentially discover more than one value for the given key. Which value would you replicate to the closest node that did not have the value? libp2p-kad can not make that choice. With a quorum of >1 Kademlia::get_record would thus not replicate the pair to the closest node that did not have the value. Instead it relied on the user first picking the right value and then calling Kademlia::put_record_to.
This pull request removes the quorum mechanism. To keep things simple, I suggest also removing the automated replication to the closest node that does not have the value, but instead entirely rely on the user to call put_record_to.
Does that make sense @dignifiedquire? Happy to expand further.
Outdated
There was a problem hiding this comment.
| /// The record found in this iteration, including the peer that returned them. | |
| /// The record found in this iteration, including the peer that returned it. |
Outdated
There was a problem hiding this comment.
I find it confusing that this is an Option. At least we need to document what None means here. Or we refactor GetRecordOk:
enum GetRecordOk {
FoundRecord (PeerRecord),
FinishedWithNoAdditionalRecord,
}
Outdated
There was a problem hiding this comment.
| /// A query initiated by [`Kademlia::get_closest_peers`]. | |
| /// A (repeated) query initiated by [`Kademlia::get_closest_peers`]. |
To be consistent with the comment updates below.
Outdated
There was a problem hiding this comment.
In my eyes tracking this as a usize is error prone and non-descriptive. Why not use ProgressStep here?
Done via cc1b2bd in case you want to cherry-pick.
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.
Thinking about this some more, this is only needed in tests. In those tests, we can get the value from the
Defaultimplementation ofKademliaConfig.I don't think we should introduce a public facing method just for testing.
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.
fixed