Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Split IO and network crates #1828

Merged
merged 4 commits into from Aug 5, 2016
Merged

Split IO and network crates #1828

merged 4 commits into from Aug 5, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Aug 3, 2016

No description provided.

@arkpar arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Aug 3, 2016
}).unwrap()
})
.expect("Error creating worker thread"));
Worker::work_loop(stealer, channel.clone(), wait, wait_mutex.clone(), deleting)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It won't panic? Or is there some other way parity will be closed in case of panic in IO Worker thread?

Copy link
Collaborator Author

@arkpar arkpar Aug 3, 2016

Choose a reason for hiding this comment

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

Set panic = "abort" in the root crate

Copy link
Contributor

Choose a reason for hiding this comment

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

panic = "abort" might not be a good idea because every panic will then lead to stuff like rocksdb handles not being closed properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fine. rocksdb and Parity are designed to be crash resistant. Removing unwinding info speeds up compilation and reduces binary size. Cargo support for this feature seems to be buggy though. Will have to revert to panic handler for now

Copy link
Contributor

Choose a reason for hiding this comment

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

i suppose we will have to keep rocksdb WAL on, though.

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 3, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Aug 3, 2016
@arkpar arkpar force-pushed the net-split branch 2 times, most recently from 5a270a2 to e26097e Compare August 3, 2016 20:57
@arkpar arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 4, 2016
@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage increased (+0.2%) to 86.632% when pulling 7634f11 on net-split into ab079fd on master.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage increased (+0.2%) to 86.689% when pulling e7efb01 on net-split into ab079fd on master.

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Aug 4, 2016
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 5, 2016
@NikVolf
Copy link
Contributor

NikVolf commented Aug 5, 2016

lgtm, no new logic here except for some error and structs mapping

@debris debris merged commit 05bfdc5 into master Aug 5, 2016
@debris debris deleted the net-split branch August 5, 2016 08:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants