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

Refactor blockchain daemon #2257

Merged
merged 1 commit into from Jul 12, 2019
Merged

Refactor blockchain daemon #2257

merged 1 commit into from Jul 12, 2019

Conversation

shal
Copy link

@shal shal commented Jul 9, 2019

  • Fix issues with timeout.
  • Split blockchains sync into threads.
  • Speed up sync using typhoeus adapter.
  • Use one BlockchainService instance per thread.
  • Use one connection per thread.

Benchmark in kovan network without deposits/withdraws: 70 blocks/sec.

@shal shal changed the title Refactor blockchain worker Refactor blockchain daemon Jul 11, 2019
app/workers/daemons/blockchain.rb Outdated Show resolved Hide resolved

logger.info { "Started processing #{bc.key} block number #{block_id}." }
def initialize
super
Copy link

Choose a reason for hiding this comment

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

I'm not sure if we still need to call super here clarify with @mnaichuk please

Copy link
Author

Choose a reason for hiding this comment

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

We need it, because we have to support graceful shutdown, that's why we use running variable.

Copy link
Author

Choose a reason for hiding this comment

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

@mnaichuk am I right, that we need to call super?

app/workers/daemons/blockchain.rb Outdated Show resolved Hide resolved
end
end

def process
@workers.map(&:join) unless running
Copy link

Choose a reason for hiding this comment

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

I think we don't need unless running here

Copy link
Author

Choose a reason for hiding this comment

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

In this way, we won't have ability to gracefully stop blockchain daemon.

Copy link
Author

Choose a reason for hiding this comment

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

We need, as I explained earlier.

@@ -0,0 +1,19 @@
version: '2'
Copy link

Choose a reason for hiding this comment

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

do we need this file ?

Copy link

Choose a reason for hiding this comment

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

We do

@@ -43,12 +44,13 @@ def json_rpc(method, params = [])
private

def connection
Faraday.new(@json_rpc_endpoint).tap do |connection|
@connection ||= Faraday.new(@json_rpc_endpoint) do |f|
f.adapter :net_http_persistent
Copy link

Choose a reason for hiding this comment

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

  1. Is it possible to explicitly define number of persistent connections ?
    Or add comment for clarifying what is default value

  2. Is it possible to write spec for this ? For being sure that it works in the way we expect

Copy link
Author

Choose a reason for hiding this comment

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

  1. I don't know how to test this kind of stuff, because we stub all calls in the tests.
  2. I think code related to gem shouldn't be tested in the peatio, it should be tested in the gem.

app/workers/daemons/blockchain.rb Outdated Show resolved Hide resolved
app/workers/daemons/blockchain.rb Show resolved Hide resolved
app/workers/daemons/blockchain.rb Show resolved Hide resolved
logger.info {"Finished processing #{bc.key} block number #{block_id}."}
end
logger.info {"Finished processing #{bc.name} blocks."}
rescue Peatio::Blockchain::ClientError => e
Copy link
Member

Choose a reason for hiding this comment

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

I would rescue StandardError here, to reduce the risk of thread death.

Copy link
Author

Choose a reason for hiding this comment

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

Agree

Copy link

@ysv ysv left a comment

Choose a reason for hiding this comment

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

TODO:

  • Rename process to run and process_block to process

  • Rename config/blockchains.yml -> e.g config/cryptonodes.yml

  • Still I'm not sure if we need unless running before joining threads

  • Add spec for Headers added by net_http_persistent

  • Add spec that faraday returns the same connection instead of initializing new one

  • Try to inspect gem for vulnerabilities for being sure

- Fix issues with timeout.
- Split blockchains sync into threads.
- Speed up sync using typhoeus adapter.
- Use one BlockchainService instance per thread.
- Use one connection per thread.
- Sleep 10 seconds if synced.
@mod mod added PR: Approved and removed PR: WIP labels Jul 12, 2019
@mod mod merged commit ddfbb04 into openware:master Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants