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

refine es_objects plugin #1271

Merged
merged 5 commits into from
Oct 15, 2018
Merged

Conversation

oxarbitrage
Copy link
Member

Most of the advances in the last time were done in the elasticsearch plugin, this pull is to make some changes at es_objects.

In this first commit:

We want to start with the most simple use case and have record of every change complicates a bit the thing. By now this is still possible to do in the plugin changing this option to false but i am also considering stuff like https://github.com/ForgeRock/es-change-feed-plugin for object changes.

  • fixed some small bugs and code cleanup.

i am currently synchronizing a testnet node with this code, have kibana access at:

http://148.251.10.231:5601/app/kibana#/discover?_g=(refreshInterval:(display:Off,pause:!f,value:0),time:(from:now-5y,mode:quick,to:now))&_a=(columns:!(_source),index:%27917a6c90-a1a7-11e8-b559-4b33a8fc253e%27,interval:auto,query:(language:lucene,query:%27_index:objects-asset%27),sort:!(block_time,desc))

and direct GET access to ES at: http://148.251.10.231:5005/

We should focus initially in this 3 indexes:

objects-asset
objects-account
object-balance

We should be able to get proxy data easy, get referer data easy, get all markets(by crossing all assets), get holder data(making use of balance object - can replace node asset_api).

@oxarbitrage
Copy link
Member Author

small update: i was syncronizing the mainnet so i chery picked the commit into the testnet and now the links are working again with testnet data:

by the way node started with:

programs/witness_node/witness_node --data-dir blockchain --rpc-endpoint "127.0.0.1:8091" --plugins "witness elasticsearch es_objects market_history" --elasticsearch-bulk-replay 1000 --elasticsearch-visitor false es-objects-bulk-replay 1000 true

kibana:
http://148.251.10.231:5601/app/kibana#/discover?_g=(refreshInterval:(display:Off,pause:!f,value:0),time:(from:now-5y,mode:quick,to:now))&_a=(columns:!(_source),index:%27917a6c90-a1a7-11e8-b559-4b33a8fc253e%27,interval:auto,query:(language:lucene,query:%27_index:objects-asset%27),sort:!(block_time,desc))

direct GET access to elasticsearch:
http://148.251.10.231:5005/

@abitmore abitmore added this to In progress in Feature release (201810) via automation Aug 23, 2018
@oxarbitrage oxarbitrage mentioned this pull request Aug 24, 2018
3 tasks
@Zapata
Copy link
Member

Zapata commented Aug 25, 2018

I think some indexes are missing:

  • objects-balance/asset_type
  • objects-balance/owner

@Zapata
Copy link
Member

Zapata commented Aug 26, 2018

Yes the fields are there but are not searchable (cf the little warn signs).

@oxarbitrage
Copy link
Member Author

in some talks with @Zapata we are thinking on 2 things to add in the context of this pull:

  • explore the possibility of merging account balances with account index and have everything in the same place similar to what get_full_account do.
  • add voting information to accounts.

@abitmore
Copy link
Member

@oxarbitrage @pmconrad @ryanRfox we need to decide whether we want this in 201810 feature release.

@pmconrad
Copy link
Contributor

IMO no.

@oxarbitrage
Copy link
Member Author

I think we should consider having this in the next release, even if still not perfect it is definitely better than the version we currently have available in master.
I still want to add some more small changes in regards to what @Zapata requested, ill resume the work here tomorrow.

prepare.clear();
}
}
std::string es_objects_plugin_impl::createIdstring(object_id_type& object_id)
{
return fc::to_string(object_id.space()) + "." + fc::to_string(object_id.type()) + "." + fc::to_string(object_id.instance());
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -47,13 +47,14 @@ class es_objects_plugin_impl
{ curl = curl_easy_init(); }
virtual ~es_objects_plugin_impl();

bool updateDatabase( const vector<object_id_type>& ids , bool isNew);
bool indexDatabase( const vector<object_id_type>& ids, std::string action);
void RemoveFromDatabase( object_id_type id, std::string index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please start method names with a lowercase letter.

I don't mind camelCase, but usually we use underscores instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should have all methods with lowercase and underscore as camel_case as part of our code styles. I changed everything to that form in fc2a9b1 and 229f215

Thanks @pmconrad

Feature release (201810) automation moved this from In progress to In Testing Oct 14, 2018
Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

As discussed previously, I haven't done a deep review since this is experimental code.
Looks ok.

@oxarbitrage oxarbitrage merged commit c532298 into bitshares:develop Oct 15, 2018
Feature release (201810) automation moved this from In Testing to Done Oct 15, 2018
@oxarbitrage oxarbitrage mentioned this pull request Oct 15, 2018
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants