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

Hybrid network support #1606

Merged
merged 18 commits into from
Oct 27, 2021
Merged

Hybrid network support #1606

merged 18 commits into from
Oct 27, 2021

Conversation

mfranciszkiewicz
Copy link
Contributor

No description provided.

@mfranciszkiewicz mfranciszkiewicz marked this pull request as ready for review October 13, 2021 13:43
@mfranciszkiewicz mfranciszkiewicz requested review from a team October 13, 2021 13:43
Copy link
Contributor

@nieznanysprawiciel nieznanysprawiciel left a comment

Choose a reason for hiding this comment

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

Good job:)
One thing that is lacking here for me, is logical splitting into functions and some code structure. For example initialization code is mixed with code for handling GSB messages and passing them to Client

Comment on lines +48 to +49
pub(crate) static ref BCAST_HANDLERS: ArcMap<String, Arc<Mutex<BCastHandler>>> = Default::default();
pub(crate) static ref BCAST_SENDER: Arc<Mutex<Option<NetSender>>> = Default::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be RwLock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's written to once, during initialization

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's why we could use RwLock to grant access from multiple threads at the same time.
But we can make this later when thinking about making whole server and client multithreaded

core/net/src/hybrid/service.rs Outdated Show resolved Hide resolved
core/net/src/hybrid/service.rs Show resolved Hide resolved
core/net/src/hybrid/service.rs Outdated Show resolved Hide resolved
core/net/src/hybrid/service.rs Show resolved Hide resolved
core/net/src/hybrid/service.rs Outdated Show resolved Hide resolved
core/net/src/hybrid/service.rs Outdated Show resolved Hide resolved
Comment on lines +490 to +496
let inner = state.inner.borrow();
inner
.services
.iter()
.find(|&id| address.starts_with(id))
// replaces /net/<dest_node_id>/test/1 --> /public/test/1
.map(|s| address.replacen(s, net::PUBLIC_PREFIX, 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe function on State struct?

@mfranciszkiewicz
Copy link
Contributor Author

@nieznanysprawiciel Could you elaborate on "initialization code is mixed with code for handling GSB messages and passing them to Client"?

- mod `codec`
- cleanup / address review comments
@nieznanysprawiciel
Copy link
Contributor

nieznanysprawiciel commented Oct 27, 2021

@nieznanysprawiciel Could you elaborate on "initialization code is mixed with code for handling GSB messages and passing them to Client"?

Sorry for not responding, I see you fixed everything already.

For example here you made elegant solution, that keeps initialization and handling separate:

tokio::task::spawn_local(broadcast_handler(brx));

When I read initialization code, I want to see, what futures we are spawning, but I don't want to see, what they are doing. On the other side, when I'm reading these handlers I want to see what they are doing, but I don't want to look for them in context of whole initialization code, because it is harder to read.

This is mainly problems with inlined closures passed to tokio::spawn, that results in mega functions containing everything.

@nieznanysprawiciel nieznanysprawiciel merged commit 6df78bb into master Oct 27, 2021
@nieznanysprawiciel nieznanysprawiciel deleted the mf/hybrid-net branch October 27, 2021 17:56
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.

Implement local broadcasts in yagna using hybrid NET
3 participants