Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: change bool to enum for is_key_within_radius_and_unavailable for better errors #974

Merged
merged 2 commits into from
Oct 10, 2023
Merged

chore: change bool to enum for is_key_within_radius_and_unavailable for better errors #974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb68438
chore: change bool to enum for is_key_within_radius_and_unavailable f…
KolbyML Oct 10, 2023
3baea03
resolve pr comments
KolbyML Oct 10, 2023
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
18 changes: 13 additions & 5 deletions portalnet/src/overlay_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ use tracing::{debug, error, info, trace, warn};
use utp_rs::{conn::ConnectionConfig, socket::UtpSocket, stream::UtpStream};

use crate::events::EventEnvelope;
use crate::storage::ShouldWeStoreContent;
use crate::{
discovery::Discovery,
events::OverlayEvent,
Expand Down Expand Up @@ -1203,6 +1204,7 @@ where
.store
.read()
.is_key_within_radius_and_unavailable(key)
.map(|value| matches!(value, ShouldWeStoreContent::Store))
.map_err(|err| {
OverlayRequestError::AcceptError(format!(
"Unable to check content availability {err}"
Expand Down Expand Up @@ -1630,10 +1632,10 @@ where
}
metrics.report_validation(true);

// Check if data should be stored, and store if true.
// Check if data should be stored, and store if it is within our radius and not already stored.
let key_desired = store.read().is_key_within_radius_and_unavailable(&key);
match key_desired {
Ok(true) => {
Ok(ShouldWeStoreContent::Store) => {
if let Err(err) = store.write().put(key.clone(), &content_value) {
warn!(
error = %err,
Expand All @@ -1642,10 +1644,16 @@ where
);
}
}
Ok(false) => {
Ok(ShouldWeStoreContent::NotWithinRadius) => {
warn!(
content.key = %key.to_hex(),
"Accepted content outside radius or already stored"
"Accepted content outside radius"
);
}
Ok(ShouldWeStoreContent::AlreadyStored) => {
warn!(
content.key = %key.to_hex(),
"Accepted content already stored"
);
}
Err(err) => {
Expand Down Expand Up @@ -1856,7 +1864,7 @@ where
.read()
.is_key_within_radius_and_unavailable(&content_key)
{
Ok(val) => val,
Ok(val) => matches!(val, ShouldWeStoreContent::Store),
Err(msg) => {
error!("Unable to read store: {}", msg);
false
Expand Down
47 changes: 33 additions & 14 deletions portalnet/src/storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ pub enum ContentStoreError {
ContentKey(#[from] ContentKeyError),
}

/// An enum which tells us if we should store or not store content, and if not why for better errors.
#[derive(Debug, PartialEq)]
pub enum ShouldWeStoreContent {
Copy link
Member

Choose a reason for hiding this comment

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

A docstring would be valuable for this struct. I'm also not in love with the name but I can't come up with better alternatives.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing that stands out to me about the name is that it is conflating two things:

  1. current status of the content
  2. the proposed action as a result of that content status

There might be a variety of reasons that callers want to know the current status of the content, besides just "should we store it". Notice the function that returns it isn't called should_store(), because there might be other reasons you want to know this info. I opened a PR with a recommended name change: #983

All that said, this PR clearly is a great improvement from the status quo.

Store,
NotWithinRadius,
AlreadyStored,
}

/// A data store for Portal Network content (data).
pub trait ContentStore {
/// Looks up a piece of content by `key`.
Expand All @@ -84,7 +92,7 @@ pub trait ContentStore {
fn is_key_within_radius_and_unavailable<K: OverlayContentKey>(
&self,
key: &K,
) -> Result<bool, ContentStoreError>;
) -> Result<ShouldWeStoreContent, ContentStoreError>;

/// Returns the radius of the data store.
fn radius(&self) -> Distance;
Expand Down Expand Up @@ -154,13 +162,15 @@ impl ContentStore for MemoryContentStore {
fn is_key_within_radius_and_unavailable<K: OverlayContentKey>(
&self,
key: &K,
) -> Result<bool, ContentStoreError> {
) -> Result<ShouldWeStoreContent, ContentStoreError> {
let distance = self.distance_to_key(key);
if distance > self.radius {
return Ok(false);
return Ok(ShouldWeStoreContent::NotWithinRadius);
}

Ok(!self.contains_key(key))
if self.contains_key(key) {
return Ok(ShouldWeStoreContent::AlreadyStored);
}
Ok(ShouldWeStoreContent::Store)
}

fn radius(&self) -> Distance {
Expand Down Expand Up @@ -228,15 +238,18 @@ impl ContentStore for PortalStorage {
fn is_key_within_radius_and_unavailable<K: OverlayContentKey>(
&self,
key: &K,
) -> Result<bool, ContentStoreError> {
) -> Result<ShouldWeStoreContent, ContentStoreError> {
let distance = self.distance_to_key(key);
if distance > self.radius {
return Ok(false);
return Ok(ShouldWeStoreContent::NotWithinRadius);
}

let key = key.content_id();
let is_key_available = self.db.get_pinned(key)?.is_some();
Ok(!is_key_available)
if is_key_available {
return Ok(ShouldWeStoreContent::AlreadyStored);
}
Ok(ShouldWeStoreContent::Store)
}

fn radius(&self) -> Distance {
Expand Down Expand Up @@ -1181,15 +1194,21 @@ pub mod test {

// Arbitrary key within radius and unavailable.
let arb_key = IdentityContentKey::new(node_id.raw());
assert!(store
.is_key_within_radius_and_unavailable(&arb_key)
.unwrap());
assert_eq!(
store
.is_key_within_radius_and_unavailable(&arb_key)
.unwrap(),
ShouldWeStoreContent::Store
);

// Arbitrary key available.
let _ = store.put(arb_key.clone(), val);
assert!(!store
.is_key_within_radius_and_unavailable(&arb_key)
.unwrap());
assert_eq!(
store
.is_key_within_radius_and_unavailable(&arb_key)
.unwrap(),
ShouldWeStoreContent::AlreadyStored
);
}

#[test]
Expand Down