-
Notifications
You must be signed in to change notification settings - Fork 648
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
database api changes in witnesses, committee members and workers calls. #352
database api changes in witnesses, committee members and workers calls. #352
Conversation
An account can own multiple workers but only one witness and one committee member, it's different. Witnesses and committee members are currently permanent, but workers may expire, so the new API's for workers (count and etc) don't make much sense, instead, it's better to have time range queries for workers. By the way, current implementation of lookup APIs are inefficient, however it's not a big deal right now because we don't have many witnesses or committee members. |
it is considered, check the worker by account sample, it was returning 2 workers for the requested account. if it is expired/active/not started/etc can be decided client side with the dates. the |
we discussed this on telegram and it is true that we can just have 1 call per section and it will be probably enough with our current network basically because we do not have a lot of records in this places. so a problem is that we need to keep the calls that are already there, for instance witness haves the 4 calls, committee had 3 and only the count was added. workers just had 1 so 3 calls were added. i need at least to add the let me know @abitmore how do you think we may proceed here. |
vector<optional<worker_object>> result; | ||
const auto& workers_idx = _db.get_index_type<worker_index>().indices().get<by_account>(); | ||
|
||
for( const auto& w : workers_idx ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid iterating through the whole index. Use lower_bound
and check for end while iterating.
libraries/app/database_api.cpp
Outdated
for (const worker_object& worker : workers_by_id) | ||
if (auto account_iter = _db.find(worker.worker_account)) | ||
if (account_iter->name >= lower_bound_name) // we can ignore anything below lower_bound_name | ||
workers_by_account_name.insert(std::make_pair(account_iter->name, worker.id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned before, this implementation is inefficient. I'd rather have a simple API to list all worker objects right now, or only the ones are active. Even range query with an account_id
lower bound is better. If we really want to query with an account_name
lower bound, best redundantly add the account_name
field into worker_object
.
auto itr = idx.find(account); | ||
vector<worker_object> result; | ||
|
||
if( itr != idx.end() && itr->worker_account == account ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a bug here, if
should be while
. Fixed in this PR.
added |
removed optional. |
In the database api there are some functions around witness, worker and committee_member objects. objects are very similar but the calls are inconsistent.
This is one of many of the problems the api_database haves described at: #95
The witness object is currently the one with more number of calls and they are:
The pull makes the 4 same calls for the other objects so in addition to the 4 witness calls we now have:
Some were already there, others were added.
Testing was done while syncing a chain at different states, your numbers will vary from the results presented here.
the 3 counters:
get witnesses:
get committee_members:
get workers:
get witness by account:
committee by account:
worker by account:
/////////// the 3 lookups:
more discussion about database_api improves can be found at: #95