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

Socket overhaul #128

Open
wants to merge 76 commits into
base: master
Choose a base branch
from
Open

Socket overhaul #128

wants to merge 76 commits into from

Conversation

mzhong1
Copy link
Collaborator

@mzhong1 mzhong1 commented Mar 5, 2020

Completely overhaul the Socket system to use STREAM sockets instead of REQ/REP sockets, adding in multithreading support for the disk-manager, and message queue handling for bynar and disk-manager.

This is in an effort to support disk-manager operations that may take more than a few minutes to execute, allowing Bynar to run operations without repeating and without hanging on a single operation for possibly hours depending on the operation.

…s with threadPool for threaded worker add_disk operations
…, added function to parse message properly and drain message bytes properly
…e partitions on the disks to be replaced to the list
src/backend/ceph.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/disk_manager.rs Outdated Show resolved Hide resolved
@0X1A
Copy link
Collaborator

0X1A commented Mar 26, 2020

I'll just post here what we went over on Slack as to include information for others or in case we need it

  • Since the ZQM polling functions can be considered non-blocking we can rule out any need for ensuring the socket polling does not spin the CPU resources at 100%
  • We should ideally begin to rule out allowing single character variables, as this impedes readability and doesn't give enough context for understanding the code at a glance
  • We understood that we currently do not have adequate tests in order to ensure proper functionality, so code reviews can only catch a certain number of issues that may arise with introducing changes.
  • Any variables that aren't quite needed should be replaced with _ or removed completely.

Other than my comment for the magic number and what I've outlined above, this LGTM

@cholcombe973
Copy link
Collaborator

Well just because it's non blocking doesn't mean it won't soak up a CPU core. I've made that mistake before and it helps to insert a small sleep to let the CPU do something else

@0X1A
Copy link
Collaborator

0X1A commented Mar 31, 2020

@cholcombe973 I initially brought up the same thing, but reluctantly decided we may not need to ensure that resource gets dunked on. That being said, weird stuff happens, and that weird stuff tends to be the stuff that bites back.

@mzhong1 I think we should probably completely promise that thread won't get blocked in the loop

src/util.rs Outdated Show resolved Hide resolved
@cholcombe973
Copy link
Collaborator

just a suggestion at this point but I think this should either be merged or split up. It's quite large

@mzhong1
Copy link
Collaborator Author

mzhong1 commented May 18, 2020

just a suggestion at this point but I think this should either be merged or split up. It's quite large

just a suggestion at this point but I think this should either be merged or split up. It's quite large

Yups, I figure at this point I will probably squash merge this if everyone is alright with this PR....

@cholcombe973
Copy link
Collaborator

Go for it

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.

4 participants