Skip to content
This repository has been archived by the owner on Jun 25, 2021. It is now read-only.

Accumulator #576

Merged
merged 5 commits into from
Aug 13, 2015
Merged

Accumulator #576

merged 5 commits into from
Aug 13, 2015

Conversation

inetic
Copy link
Contributor

@inetic inetic commented Aug 12, 2015

Review on Reviewable

@maidsafe-highfive
Copy link

r? @benjaminbollen

(maidsafe_highfive has picked a reviewer for you, use r? to override)

@benjaminbollen
Copy link

looks good ! great that we can indeed consider a general RoutingMessage accumulator !

@dirvine
Copy link
Member

dirvine commented Aug 12, 2015

👍 , nice one @inetic

@inetic inetic changed the title WIP: Accumulator Accumulator Aug 13, 2015
@@ -348,6 +352,11 @@ impl RoutingNode {
return Err(RoutingError::BadAuthority);
}

let (message, opt_token) = match self.accumulate(message_wrap) {

Choose a reason for hiding this comment

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

Can we exclude InternalResponse::CacheNetworkName from this accumulator; we are currently not using FindGroup/Response::FindGroup so no need to mind that; for now just don't accumulate CacheNetworkName Responses

Choose a reason for hiding this comment

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

Sorry you deal with this in fn accumulate; ignore my comment !

Choose a reason for hiding this comment

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

Sorry no :), this function does not exclude this one message type; can we patch this here?

@benjaminbollen
Copy link

looks good but can you apply the patch to exclude InternalResponse::CacheNetworkName from the accumulator (it will never resolve as every message is seen as different, as it includes the close group)

@benjaminbollen
Copy link

looks good to me

benjaminbollen pushed a commit that referenced this pull request Aug 13, 2015
@benjaminbollen benjaminbollen merged commit a80b2b1 into maidsafe:master Aug 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants