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

cmd, common, eth: cache orchestrator stake #1216

Merged
merged 3 commits into from
Dec 4, 2019
Merged

Conversation

kyriediculous
Copy link
Contributor

@kyriediculous kyriediculous commented Nov 26, 2019

What does this pull request do? Explain your changes. (required)
This PR caches the stake for orchestrators in the active set whenever a new round event is emitted.

This can be both a removed log (re-org) or an added log (new round), in both cases we make an RPC call for the current round number and a subsequent RPC call for each O to fetch its stake for that specific round. We then update the orchestrator in the DB with the new stake.

Specific updates (required)

  • Added a stake column to the orchestrators table and updated UpdateOrch and SelectOrchs methods accordingly
  • Added a RoundsWatcher subscription to OrchestratorWatcher
  • Added internal helper methods handleRoundEvent and cacheOrchestratorStake to OrchestratorWatcher
  • updated StubEthClient.GetTranscoderPoolForRound
  • Cache stake for orchestrators when DBOrchestratorPoolCache is created

How did you test each of these updates (required)
ran unit tests

Does this pull request close any open issues?
Fixes #1213

Checklist:

  • README and other documentation updated
  • Node runs in OSX and devenv
  • All tests in ./test.sh pass

@kyriediculous
Copy link
Contributor Author

Changed base branch to #1182 for now and added caching stake when starting the node.

@yondonfu
Copy link
Member

Looks like there are changes from #1182 pulled into this PR (due to the rewording of commit history in #1182). Mind rebasing on top of #1182?

@kyriediculous
Copy link
Contributor Author

dropped the commits and rebased on latest #1182

@kyriediculous kyriediculous changed the base branch from nv/poolcache-update to master December 2, 2019 17:52
}

for _, o := range orchs {
go getStake(o)
Copy link
Member

Choose a reason for hiding this comment

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

Creating a goroutine for each query feels like a premature optimization at this point IMO since it introduces a piece of code that needs to be concurrency safe (and ideally we would test for it). Caching the stake for each active O isn't that time sensitive since it just happens on startup. Additionally, a simpler optimization that we could use in the future is to write a wrapper contract that aggregates all the stake values for a list of Os so that we can fetch the results with a single query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not mistaken go-sqlite does use a RWmutex so this should be thread safe regardless, since we're only writing the value to the DB and not memory.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah good point. On second thought, I'm fine with the concurrent requests used here 👍 with the wrapper contract as a nice to have optimization later on that removes the need for any concurrency code.

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM

}

for _, o := range orchs {
go getStake(o)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah good point. On second thought, I'm fine with the concurrent requests used here 👍 with the wrapper contract as a nice to have optimization later on that removes the need for any concurrency code.

@kyriediculous kyriediculous merged commit 17c4866 into master Dec 4, 2019
@kyriediculous kyriediculous deleted the nv/cache-stake branch December 4, 2019 21:44
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.

Cache stake of all active orchestrators at when a new round is initialized
2 participants