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

WIP feat/routing: use node key as name #1115

Closed
wants to merge 1 commit into from

Conversation

maqi
Copy link
Member

@maqi maqi commented Jul 28, 2016

This change is Reviewable

@maidsafe-highfive
Copy link

r? @afck

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

@maqi maqi force-pushed the nodes_key_as_name branch 2 times, most recently from 2eb5d0c to ac14026 Compare July 29, 2016 04:24
@afck
Copy link
Contributor

afck commented Jul 29, 2016

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/xor_name.rs, line 126 [r2] (raw file):

        let mut distance = XorName([0u8; XOR_NAME_LEN]);
        for i in 0..XOR_NAME_BITS {
            distance.0[i] = self.0[i] ^ target.0[i];

That's not minus, that's xor. But for the interval length computation we'll need minus. Or rather: the absolute value of the difference. E.g. compute self - target if self >= target, otherwise return target.minus(self) instead.
And maybe it should be called abs_diff then, or euclidean_distance.


src/states/node.rs, line 979 [r2] (raw file):

        println!("input nodes {:?}", nodes);
        let name = XorName([0u8; XOR_NAME_LEN]);
        nodes.sort_by(|node0, node1| name.cmp_distance(&node0.name(), &node1.name()));

Doesn't XorName implement Ord anyway? (If not: it should, I think.) So you could probably just call nodes.sort().


src/states/node.rs, line 983 [r2] (raw file):

        let mut max_range = XorName([0u8; XOR_NAME_LEN]);
        let mut range_index = 0;
        for i in 0..(nodes.len() - 1) {

Once Disjoint Groups are in place, this will need to include the lower and upper bound of the group's range (if there are no nodes with exactly those names).
Let's either add a TODO or hold off with merging this PR until after RFC 37 is implemented.


Comments from Reviewable

@afck
Copy link
Contributor

afck commented Jul 29, 2016

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/xor_name.rs, line 126 [r2] (raw file):

Previously, afck (Andreas Fackler) wrote…

That's not minus, that's xor. But for the interval length computation we'll need minus. Or rather: the absolute value of the difference. E.g. compute self - target if self >= target, otherwise return target.minus(self) instead.
And maybe it should be called abs_diff then, or euclidean_distance.

Actually, ignore the "absolute" part of my comment: Just implementing subtraction should be fine, since you sort the names below anyway, so you always subtract the smaller from the larger one. I think we should just `impl std::ops::Sub for XorName` then: implement subtraction for 256-bit integers.

Comments from Reviewable

@maqi
Copy link
Member Author

maqi commented Aug 2, 2016

src/states/node.rs, line 979 [r2] (raw file):

Previously, afck (Andreas Fackler) wrote…

Doesn't XorName implement Ord anyway? (If not: it should, I think.) So you could probably just call nodes.sort().

we can call nodes.sort(), but it doesn't change the order (or it sorted in the decreasing order?)

Comments from Reviewable

@afck
Copy link
Contributor

afck commented Aug 2, 2016

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/states/node.rs, line 979 [r2] (raw file):

Previously, maqi wrote…

we can call nodes.sort(), but it doesn't change the order (or it sorted in the decreasing order?)

Ah, sorry, it is `PublicId` that needs to implement `Ord` - and that should definitely go by _name_. Maybe it will, if we just make `name` the first field in `PublicId`? If not, `Ord` needs to be implemented by hand.

In either case, please add a test to id.rs to make sure that public IDs are ordered by name.


Comments from Reviewable

/// routing table. If yes, and if A's ID is in its ID cache, Z sends its own `ConnectionInfo` back
/// to A and also attempts to connect to A via Crust. A does the same, once it receives the
/// For each `ConnectionInfo` that a node Z(members of Y) receives from A, it decides whether it wants A in its
/// routing table. If yes, and if A's ID is in its ID cache(node A's original ID), Z sends its own
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow... why would Z have A's original ID? For the previous line, it would be nicer if your addition was spelled out: "node Z (of NaeManager Y)"

Copy link
Member Author

Choose a reason for hiding this comment

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

the group X has notified group Y about A's original id

self.sent_network_name_to = None;
}
}
// TODO: unless Disjoin Group being implemented, it is not guaranteered cache exists
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling: "Disjoint", "guaranteed"

fn compute_new_id(&mut self, name_range: &(XorName, XorName)) -> Result<(), RoutingError> {
let now = Instant::now();
loop {
if now.elapsed() > Duration::from_secs(COMPUTING_IDENTITY_TIMEOUT_SECS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets hope the client/joining node is not busy, or it might not manage many iterations. Often this type of code allows a maximum number of tries instead of a timeout.

Copy link
Member Author

Choose a reason for hiding this comment

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

the network side is waiting based on timeout, so here needs to be wait on timeout as well.

@Viv-Rajkumar
Copy link
Contributor

closing this for now. lets get this raised and merged when switching the node name pattern with node ageing.

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

5 participants