Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
7ce7057
feat(kad): add limit option for getting providers
dignifiedquire Jun 16, 2022
769ce4b
feat(kad): report get_providers call event based
dignifiedquire Jun 16, 2022
f68c903
change `GetRecord`api
dignifiedquire Sep 27, 2022
baf3e60
apply cr
dignifiedquire Oct 4, 2022
5db1bfa
tests and fixups for getclosestpeers
dignifiedquire Nov 11, 2022
91b5f59
remove KademliaCaching
dignifiedquire Nov 11, 2022
30eb56c
apply cr: move to nonzerousize
dignifiedquire Nov 11, 2022
0720deb
fix example
dignifiedquire Nov 11, 2022
a0b26b0
happy clippy
dignifiedquire Nov 11, 2022
e0e79fd
protocols/kad: Refactor step tracking
mxinden Nov 11, 2022
a4f0210
cr feedback round 1
dignifiedquire Nov 12, 2022
4e448c4
cr: improve api for GetRecordOk and GetProvidersOk
dignifiedquire Nov 12, 2022
96a952e
fixup rebase of examples
dignifiedquire Nov 12, 2022
2724afd
switch to counter for number of observed record
dignifiedquire Nov 12, 2022
83a2a86
bring back kademliacaching
dignifiedquire Nov 12, 2022
b483983
examples/file-sharing: Revert usage of HashSet
mxinden Nov 17, 2022
aa8a6ce
examples/file-sharing: Finish query once provider is found
mxinden Nov 17, 2022
3912acc
protocols/kad: Remove pub(crate) from replication_factor
mxinden Nov 17, 2022
e339cdf
protocols/kad: Refactor get_record
mxinden Nov 17, 2022
3ced598
protocols/kad: Refactor get_providers step instantiation
mxinden Nov 17, 2022
ac2e525
protocols/kad: Remove pub from ProgressStep methods
mxinden Nov 17, 2022
5ca8d70
remove unused as_intermediary_result
dignifiedquire Nov 18, 2022
bd05da6
fix test cr comments
dignifiedquire Nov 18, 2022
c7c4341
fixup: rebase
dignifiedquire Nov 18, 2022
5ecf9cb
use get instead of checked_add
dignifiedquire Nov 18, 2022
055636e
Update protocols/kad/src/behaviour.rs
dignifiedquire Nov 18, 2022
d5cb7e9
Merge branch 'master' into feat-kad-count
dignifiedquire Nov 22, 2022
0e443d4
Merge remote-tracking branch 'upstream/master' into feat-kad-count
dignifiedquire Nov 24, 2022
c9c00df
add changelog
dignifiedquire Nov 24, 2022
05e29d4
Update protocols/kad/CHANGELOG.md
dignifiedquire Nov 24, 2022
1223f02
Update protocols/kad/CHANGELOG.md
dignifiedquire Nov 24, 2022
9c28d98
Update protocols/kad/CHANGELOG.md
dignifiedquire Nov 24, 2022
cfd5461
Merge branch 'master' into feat-kad-count
dignifiedquire Nov 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
apply cr
  • Loading branch information
dignifiedquire committed Nov 18, 2022
commit baf3e607318c98f9034781b56c9d99d2b02a66b3
2 changes: 1 addition & 1 deletion misc/metrics/src/kad.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ impl super::Recorder<libp2p_kad::KademliaEvent> for Metrics {
libp2p_kad::QueryResult::GetRecord(result) => match result {
Ok(ok) => self
.query_result_get_record_ok
.observe(ok.records.len() as f64),
.observe(ok.record.as_ref().map(|_| 1).unwrap_or_default() as f64),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to still track query_result_get_record_ok as a histogram if the only values are 0 and 1? Wouldn't a Counter make more sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it does, changed

Err(error) => {
self.query_result_get_record_error
.get_or_create(&error.into())
Expand Down
39 changes: 9 additions & 30 deletions protocols/kad/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -706,12 +706,12 @@ where
let info = QueryInfo::GetRecord {
key,
count: 1,
record_to_cache: record.as_ref().map(|r| r.record.clone()),
found_a_record: record.is_some(),
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);

// No queries were actually done for the results yet.
let stats = QueryStats::empty();
Expand Down Expand Up @@ -1381,29 +1381,10 @@ where
QueryInfo::GetRecord {
key,
count,
record_to_cache,
found_a_record,
cache_candidates,
} => {
let results = if let Some(record) = record_to_cache {
// [not empty]
if !cache_candidates.is_empty() {
// Cache the record at the closest node(s) to the key that
// did not return the record.
let quorum = NonZeroUsize::new(1).expect("1 > 0");
let context = PutRecordContext::Cache;
let info = QueryInfo::PutRecord {
context,
record,
quorum,
phase: PutRecordPhase::PutRecord {
success: vec![],
get_closest_peers_stats: QueryStats::empty(),
},
};
let inner = QueryInner::new(info);
self.queries
.add_fixed(cache_candidates.values().copied(), inner);
}
let results = if found_a_record {
Ok(GetRecordOk {
record: Default::default(),
cache_candidates,
Expand Down Expand Up @@ -2238,13 +2219,11 @@ where
key,
ref mut count,
cache_candidates,
ref mut record_to_cache,
ref mut found_a_record,
} = &mut query.inner.info
{
if let Some(record) = record {
if record_to_cache.is_none() {
*record_to_cache = Some(record.clone());
}
*found_a_record = true;
let record = PeerRecord {
peer: Some(source),
record,
Expand Down Expand Up @@ -2675,7 +2654,7 @@ pub type GetRecordResult = Result<GetRecordOk, GetRecordError>;
/// The successful result of [`Kademlia::get_record`].
#[derive(Debug, Clone)]
pub struct GetRecordOk {
/// The record found in this iteration, including the peer that returned them.
/// The record found in this iteration, including the peer that returned it.
pub record: Option<PeerRecord>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, fixed

/// If caching is enabled, these are the peers closest
/// _to the record key_ (not the local node) that were queried but
Expand Down Expand Up @@ -3027,8 +3006,8 @@ pub enum QueryInfo {
key: record::Key,
/// Current index of events.
count: usize,
/// The record that is to be cached in the `cache_candidates`.
record_to_cache: Option<Record>,
/// Did we find at least one record?
found_a_record: bool,
/// The peers closest to the `key` that were queried but did not return a record,
/// i.e. the peers that are candidates for caching the record.
cache_candidates: BTreeMap<kbucket::Distance, PeerId>,
Expand Down