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.

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

feature/broadcast-trees #89

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

feature/broadcast-trees #89

wants to merge 29 commits into from

Conversation

decanus
Copy link
Contributor

@decanus decanus commented May 5, 2020

closes #88 #51 #56

This PR is currently in draft.

  • Subscribe Function for node
  • Unsubscribe Function for node
  • Subscription handling when called by a child
  • Unsubscription handling when called by a child
  • Handling when a child disconnects
  • Handling when a parent disconnects
  • Finding a parent to subscribe to
  • Unsubscribing from parent
  • Message forwarding to children
  • Message definitions for SUBSCRIBE, UNSUBSCRIBE and regular MESSAGES

What we solve here is how to construct the tree, however we do not solve how messages would be distributed as we can't always directly contact the root.

Also note: Do we need to drop messages that come from a parent who we do not recognize as our parent? Or better said, do we drop messages from peers who are neither our children or parent?

We also need IDs, the current way the Addr works using the Bluetooth Mac will not work. We need the device to have the same ID for all it’s peers.

Another note, when trying to find a parent for a specific node we need to then check if self is closer than parent, if it is self is the parent.

Sources/UB/Types.swift Outdated Show resolved Hide resolved
Sources/UB/Types.swift Outdated Show resolved Hide resolved
Sources/UB/Types.swift Outdated Show resolved Hide resolved
Sources/UB/Types.swift Outdated Show resolved Hide resolved
Sources/UB/Types.swift Outdated Show resolved Hide resolved
Sources/UB/Types.swift Show resolved Hide resolved
Sources/UB/Types.swift Show resolved Hide resolved
Sources/UB/Types.swift Show resolved Hide resolved
Sources/UB/Types.swift Show resolved Hide resolved

func closest(to: Addr) -> Addr? {

var distance = 0;
Copy link

Choose a reason for hiding this comment

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

Lines should not have trailing semicolons.

Sources/UB/Node.swift Show resolved Hide resolved
Sources/UB/Node.swift Show resolved Hide resolved
Sources/UB/Node.swift Outdated Show resolved Hide resolved
Sources/UB/Node.swift Outdated Show resolved Hide resolved
Sources/UB/Node.swift Show resolved Hide resolved
Sources/UB/Node.swift Show resolved Hide resolved
@decanus
Copy link
Contributor Author

decanus commented May 5, 2020

We need to ensure that the last node in the tree, or a node without a “parent” sends the message to all its children. AKA we need to remove the exclude.

@ultralight-beam ultralight-beam deleted a comment from codeclimate bot May 5, 2020
Tests/UBTests/NodeTests.swift Outdated Show resolved Hide resolved
Tests/UBTests/NodeTests.swift Outdated Show resolved Hide resolved
Tests/UBTests/NodeTests.swift Outdated Show resolved Hide resolved
Tests/UBTests/NodeTests.swift Outdated Show resolved Hide resolved
Tests/UBTests/NodeTests.swift Outdated Show resolved Hide resolved

XCTAssertEqual(0, transport.sent.count)
let data = try! packet.serializedData()
Copy link

Choose a reason for hiding this comment

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

Force tries should be avoided.

origin: Addr(repeating: 3, count: 3),
message: Data(repeating: 0, count: 3)
)
let subscription = try! subscribe.serializedData()
Copy link

Choose a reason for hiding this comment

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

Force tries should be avoided.


XCTAssertEqual(2, transport.sent.count)
let data = try! unsubscribe.serializedData()
Copy link

Choose a reason for hiding this comment

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

Force tries should be avoided.


node.send(message)
let subscription = try! subscribe.serializedData()
Copy link

Choose a reason for hiding this comment

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

Force tries should be avoided.

@GregTheGreek
Copy link

code climate is violent, pr looks good, still need to review it further

guard let data = try? message.toProto().serializedData() else {
let packet = Packet.new(topic: Data(topic), type: .message, body: data)
guard let data = try? packet.serializedData() else {
// @todo error

Choose a reason for hiding this comment

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

please panic or log

@ultralight-beam ultralight-beam deleted a comment from GregTheGreek May 14, 2020
var distance = 0
var addr: Addr?

forEach { peer in
Copy link
Member

Choose a reason for hiding this comment

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

Can just call min(by:) https://developer.apple.com/documentation/swift/array/2298201-min

Much more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh true

ec2
ec2 previously approved these changes May 18, 2020

extension Array where Element == Addr {
func closest(to: Addr) -> Addr? {
return self.min { a, b in
Copy link

Choose a reason for hiding this comment

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

Variable name should be between 3 and 40 characters long: 'a'


extension Array where Element == Addr {
func closest(to: Addr) -> Addr? {
return self.min { a, b in
Copy link

Choose a reason for hiding this comment

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

Variable name should be between 3 and 40 characters long: 'b'

@codeclimate
Copy link

codeclimate bot commented May 20, 2020

Code Climate has analyzed commit 6813cdf and detected 0 issues on this pull request.

View more on Code Climate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better message routing
3 participants