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

Issue #554 - Adding Farmers::Fork #567

Merged
merged 8 commits into from Nov 27, 2018
Merged

Conversation

emilianodellacasa
Copy link
Contributor

To solve issue #554 I added the new class Farmers::Fork that is using Process.fork instead of Open3

Please note that inside the library, Farmers::Spawn is still the default

@0crat 0crat added the scope label Nov 22, 2018
@0crat
Copy link
Collaborator

0crat commented Nov 22, 2018

Job #567 is now in scope, role is REV

@emilianodellacasa emilianodellacasa mentioned this pull request Nov 22, 2018
@0crat
Copy link
Collaborator

0crat commented Nov 22, 2018

@yegor256/z everybody who has role REV is banned at #567; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@yegor256
Copy link
Collaborator

@emilianodellacasa what's the point of doing fork and creating a new process? :)

@yegor256
Copy link
Collaborator

@emilianodellacasa you are saying "instead" but your code still calls Open3.popen2e

@codecov-io
Copy link

codecov-io commented Nov 22, 2018

Codecov Report

Merging #567 into master will decrease coverage by 0.2%.
The diff coverage is 27.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
- Coverage   34.62%   34.42%   -0.21%     
==========================================
  Files          65       65              
  Lines        2992     3027      +35     
==========================================
+ Hits         1036     1042       +6     
- Misses       1956     1985      +29
Impacted Files Coverage Δ
lib/zold/node/farmers.rb 28.39% <27.27%> (-4.26%) ⬇️
lib/zold/sync_wallets.rb 47.36% <0%> (-2.64%) ⬇️
lib/zold/remotes.rb 30.81% <0%> (-0.63%) ⬇️
lib/zold/node/async_entrance.rb 28.57% <0%> (-0.52%) ⬇️
lib/zold/commands/node.rb 19.09% <0%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22ea23b...16337b4. Read the comment docs.

@emilianodellacasa
Copy link
Contributor Author

@yegor256 Ooooops, sorry about that, now the code makes sense :-)

@yegor256
Copy link
Collaborator

@emilianodellacasa still a miss. You don't need to use posix processes inside fork. Fork already creates you a separate process. Why do you want to create another one inside?

@emilianodellacasa
Copy link
Contributor Author

@yegor256 I don't get it: I need a separate process to run the command that calculates the score, this is the reason why I am using another process for it.

@yegor256
Copy link
Collaborator

yegor256 commented Nov 23, 2018

@emilianodellacasa you get that process by calling fork. Then, you just calculate the score inside the method. Just like it's done in Farmers::Plain:

class Fork
  def up(score)
    fork do
      score.next
    end
  end
end

@emilianodellacasa
Copy link
Contributor Author

@yegor256 You are definitely right, I was missing the whole concept of score.next

write.close
output = read.read
buffer = output.split('|')[0]
proc_pid = output.split('|')[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yegor256 Done


def up(score)
start = Time.now
read, write = IO.pipe
Copy link
Collaborator

Choose a reason for hiding this comment

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

@emilianodellacasa I would rename them to stdin and stdout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yegor256 Done

Process.wait
write.close
output = read.read
buffer = output.split('|')[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@emilianodellacasa you can do:

buffer, pid = output.split('|')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yegor256 Right, done

@yegor256
Copy link
Collaborator

@emilianodellacasa see above

* upstream/master:
  zold-io#572 tested and fixed
  Issue zold-io#537 - Test modified for pushing
  Issue zold-io#537 - Test modified for fecthing
  Issue zold-io#537 - Fixing rubocop code smells
  Change link of travis build status to master branch in README
  Issue zold-io#537 - --threads for PUSH and FETCH
  zold-io#568 new location of lock files
  zold-io#568 Remotes thread safety
  zold-io#568 --queue-limit
  zold-io#566 fixed
  zold-io#563 --no-cache
  Isse zold-io#553 - Excluding memory-dump from test
@emilianodellacasa
Copy link
Contributor Author

@yegor256 All done, see my commits

@yegor256
Copy link
Collaborator

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Nov 27, 2018

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit d58afc5 into zold-io:master Nov 27, 2018
@rultor
Copy link
Collaborator

rultor commented Nov 27, 2018

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 8min)

@0crat 0crat removed the scope label Nov 27, 2018
@0crat
Copy link
Collaborator

0crat commented Nov 27, 2018

Job gh:zold-io/zold#567 is not assigned, can't get performer

@0crat
Copy link
Collaborator

0crat commented Nov 27, 2018

The job #567 is now out of scope

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.

None yet

5 participants