es_objects plugin #500

oxarbitrage · 2017-11-23T23:49:15Z

Plugin will store certain objects from the blockchain to ES database. Can be easy extended to support more objects. Initial idea was to store proposal objects for #115

Instead of loading the database with more data i decided to store the proposal_objects to ES where no object is deleted while we leave the core to keep with the reduced active proposal list.

I noticed then that we could store account data so we can go over #452 without loading more the blockchain database with a working referral index. By saving the account data we can index by referrer with ease(ill post some samples). Another advantage is in the case we want to get the proxies, currently we have to loop throw all the accounts and checking the options.vote_as field. Very intensive.

Asset and balance objects were also added to the plugin to solve similar issues and make the searching inside them easier for the client apps.

Runs good among the original elasticsearch plugin for operations.A starting command with the 2 plugins can be something as:

programs/witness_node/witness_node --data-dir data/my-blockprod --rpc-endpoint "127.0.0.1:8090" --plugins "elasticsearch es_objects" --es-objects-proposals true --es-objects-accounts true --es-objects-assets true --es-objects-balances true

Please let me know what do you guys think.

abitmore

I slightly tend to merge this plugin into the earlier ES plugin.

abitmore · 2017-11-24T10:42:06Z

libraries/plugins/es_objects/es_objects.cpp

+   acct.lifetime_referrer_fee_percentage = account_object->lifetime_referrer_fee_percentage;
+   acct.referrer_rewards_percentage = account_object->referrer_rewards_percentage;
+   acct.name = account_object->name;
+   acct.owner = fc::json::to_string(account_object->owner.get_addresses());


Better use the whole account_object->owner here. Addresses are only used for backward compatibility. Usually people use keys and account ids.
This also applies to active authorities.

added in this commit 3545c22
i had to add the whole flat_map in text form to make it work. this will make the indexing by this fields hard but i think its ok, the real intention is to have something available to relate the accounts index with the balances that uses an owner key.

abitmore · 2017-11-24T10:42:29Z

libraries/plugins/es_objects/es_objects.cpp

+   acct.name = account_object->name;
+   acct.owner = fc::json::to_string(account_object->owner.get_addresses());
+   acct.active = fc::json::to_string(account_object->active.get_addresses());
+   acct.voting_account = account_object->options.voting_account;


I think it's better to use a constructor function to initialize all these fields.

pls explain a bit more this because i don't get it. constructor will be made in the hpp while i don't have this values available there to initialize this way, i am probably missing something, an example may be good. thanks.

For example, in hpp file, have

struct account_struct { string name; ... account_struct(const account_object* obj) { name = obj->name; ... } }

In cpp file (here), only one line:

account_struct a(pointer_to_the_account_obj);

abitmore · 2017-11-24T10:46:49Z

libraries/plugins/es_objects/es_objects.cpp

+}
+
+void es_objects_plugin_impl::SendBulk()
+{


This looks like duplicate code. Better abstract to dedicated utility class, so can be used in all ES related plugins.

agree, working on it.

abitmore · 2017-11-24T10:48:01Z

libraries/plugins/es_objects/es_objects.cpp

+   prop.id = proposal_object->id;
+   prop.expiration_time = proposal_object->expiration_time;
+   prop.review_period_time = proposal_object->review_period_time;
+   prop.proposed_transaction = fc::json::to_string(proposal_object->proposed_transaction);


IMHO we should store approved_by fields for proposal objects.

can it be done by adding the proposal objects fields required_active_approvals, available_active_approvals, required_owner_approvals, available_owner_approvals and available_key_approvals ?
let me know if this will do it so i can add these.

Yes. It worth a try, although I'm not sure whether the final state can be correctly stored with current implementation, because if the proposal has no review period, the object will be removed in the same block when the proposal is approved.

added on fe4e87b

abitmore · 2017-11-24T10:58:30Z

libraries/plugins/es_objects/es_objects.cpp

+   }
+
+   for(auto const& value: ids) {
+      if(value.space() == 1 && value.type() == 10 && _es_objects_proposals) {


Don't use magic numbers. We can check like this:

if( value.is<proposal_object>() && _es_objects_proposals )

yes, sorry, i was supposed to change that before sending the pull. changed now here 26c57fd

abitmore · 2017-11-24T10:59:23Z

libraries/plugins/es_objects/es_objects.cpp

+
+   for(auto const& value: ids) {
+      if(value.space() == 1 && value.type() == 10 && _es_objects_proposals) {
+         auto obj = db.find_object(value);


This can be nullptr.

added check for nullptr at 7b18456

abitmore · 2017-11-24T11:02:47Z

libraries/plugins/es_objects/es_objects.cpp

+      bool _es_objects_assets = true;
+      bool _es_objects_balances = true;
+      bool _es_objects_logs = true;
+      CURL *curl; // curl handler


Do we want to use a new curl handler for each plugin?

interesting question, this is debatable at least to me. using the same handler for all requests that goes to the same url is recommended for performance and doing otherwise will be impossible for our purposes in the original plugin(connect and disconnect with each request will be just too slow in a replay).

i made a new handler here because i am not sure how much is too much when they run at the same time. the performance of sending bulks in 5k operation batches were proven to be good enough in a replay scenario while if we add more queries to the same handler we could run into performance issues with more data coming in by new plugins trying to store more data in the database. for example if first plugin is sending a bulk of 5k operations(10000 lines of text ) and this new plugin is trying to send 1k lines more it will need to wait until the handler is available to do it ending in some delay that i am not sure if it will be a problem in practice.

basically for that reason a new handler was added and answering the question; yes. the intention was to use a new handler with each plugin so they can be sending data in "parallel".

i was able to get in sync a node with the 2 plugins running at the same time and it was ok this way, i can make some tests by using the same handler and check that if you think it worth it.

Actually I'm not sure, so was asking.
If the 2 plugins are merged into one, there will be only one handler by nature.

abitmore · 2017-11-24T11:06:25Z

libraries/plugins/es_objects/es_objects.cpp

+         auto obj = db.find_object(value);
+         auto b = static_cast<const balance_object*>(obj);
+         PrepareBalance(b);
+      }


It's best if we can also store account statistics, dynamic asset data, bitasset data and etc along with the main objects.

abitmore · 2017-11-24T11:07:47Z

libraries/plugins/es_objects/include/graphene/es_objects/es_objects.hpp

+// time.
+//
+#ifndef PROPOSAL_SPACE_ID
+#define PROPOSAL_SPACE_ID 6


Better use a better name here.
Also I'm not sure if the number 6 is appropriate.

this is something i dont understand fully so maybe you can explain me a bit. the comment states that plugins with the same ID will be added to the same binary so i think maybe is appropriate to have the same ID in the 2 ES plugins as: https://github.com/bitshares/bitshares-core/blob/develop/libraries/plugins/elasticsearch/include/graphene/elasticsearch/elasticsearch_plugin.hpp#L47

but maybe i am wrong, the snapshot plugin from @pmconrad actually dont define a number(https://github.com/bitshares/bitshares-core/blob/develop/libraries/plugins/snapshot/include/graphene/snapshot/snapshot.hpp) please let me know if we should change this pulgin to number 7.

in regards to the better name i am not sure what do you mean, there is no name defined here but only the ID. let me know. thanks @abitmore !

I was referring that you use es_object for the plugin name but setting PROPOSAL here, it's not good practice. Not about the number.
According to the comment above, there should be a script to change the ID's at building time, but I'm not sure if there is one.
If I understood correctly, the space ID's are used to define a "space" for data storing in the object database. So, if no new object type is defined and need to be stored into object database, no need to set a XX_SPACE_ID, as have done with the snapshot plugin.

oh, sorry about that name, didn't saw it. it must be there because in a first version i was only going to store proposals. anyways, with your clarification i now removed all that section from the hpp file in commit 7b453c6 and code stills compiles and works fine.
thanks.

oxarbitrage · 2017-11-30T18:07:55Z

I slightly tend to merge this plugin into the earlier ES plugin.

After thinking about this i tend to agree and think it could be a good idea to have a single plugin with all the elastic stuff on it. I can close this pull and try to add this to the original if you think it will be better, i don't have any valid argument to don't do it that way neither against multiple plugins so i am not sure.

pnomolos · 2018-02-25T06:06:22Z

Hi @oxarbitrage What's the time frame on this do you think? Thanks!

oxarbitrage · 2018-02-25T19:19:31Z

last commit (2bf302d) moves SendBulk and createBulk common elasticsearch calls to the utilities part of the project. es_objects plugin now make uses of them and also original elasticsearch_plugin can use them but i will make that in another pull.

it also add some console output for errors(#681), add elasticsearch 6 support and remove unnecessary includes.

oxarbitrage · 2018-02-25T20:28:06Z

Hi @oxarbitrage What's the time frame on this do you think? Thanks!

I have resumed to work on it to have it included ASAP.

xeroc · 2018-03-02T09:58:50Z

libraries/plugins/es_objects/es_objects.cpp

+
+      graphene::chain::database& database()
+      {
+         return _self.database();


What's this for?

no need, simplified database call here 207eb07 thanks!

xeroc · 2018-03-02T10:01:19Z

libraries/plugins/es_objects/es_objects.cpp

+
+   // check if we are in replay or in sync and change number of bulk documents accordingly
+   uint32_t limit_documents = 0;
+   if((fc::time_point::now() - block_time) < fc::seconds(30))


Is this the only way to figure out if we are in replay or normal mode?

not that i know, this one seems to be effective as it is used in already implemented elasticsearch for account history plugin: https://github.com/bitshares/bitshares-core/blob/master/libraries/plugins/elasticsearch/elasticsearch_plugin.cpp#L198

of course open to ideas to change both.

xeroc · 2018-03-02T10:03:12Z

libraries/plugins/es_objects/es_objects.cpp

+}
+
+void es_objects_plugin::plugin_startup()
+{


Can you add a log entry here so people have feedback about es plugin being enabled when starting the backend, please?

added at 109b19f thx.

xeroc · 2018-03-02T10:04:37Z

Looks great!

…into elasticsearch-extras

oxarbitrage · 2018-03-13T22:32:56Z

when saving object bitasset i realized it hits the database hard, even if feed and current_feed don't change the object is updated with a new date all the time, plus updated when any of the feeds inside feed change and finally also updates when current_feed change. this last and only this is what we want to track.

so i created a map of bitassets to keep track of the last current_feed string in order to compare when object change and insert records in the database only in that situation.

i didn't synchronized the chain full but it seems to be reasonable. maintaining the map of bitassets will increase RAM but it is better than massively querying the ES database to check if a change in current feed was done.

abitmore

At a glance it's fine. We can merge it.

abitmore · 2018-03-19T20:45:21Z

libraries/plugins/es_objects/es_objects.cpp

+
+void es_objects_plugin_impl::PrepareAsset(const asset_object* asset_object)
+{
+   asset_struct asset;


I'd recommend not use "asset" as a local variable name, since there is a class or struct has same name. I'm not sure if it will cause trouble.

…me a const

oxarbitrage · 2018-03-20T01:41:16Z

don't merge yet please, need a few more changes.

abitmore · 2018-03-20T09:32:50Z

This discussion is interesting: steemit/steem#2191

oxarbitrage · 2018-03-20T12:45:42Z

i am having a problem with the limit orders saving. it seems that when limit and fill are in the same block order 1.7.X is not created. the market history plugin subscribes to applied_block, maybe i that one at least for limit orders.

This discussion is interesting: steemit/steem#2191

will check this.

abitmore · 2018-03-20T13:07:34Z

When a new limit order is filled in the same block, it will be created then removed, so you can't get the limit order with db.get(...) after the block is applied. The observer mechanism used in #732 may help.

abitmore

We can merge this now, and fix the same-block-removal issue with another PR.

pnomolos · 2018-03-20T20:42:28Z

@oxarbitrage I should now be able to go ahead with bitshares/bitshares-ui#661, right?

oxarbitrage · 2018-03-20T20:48:24Z

@pnomolos close to it but not yet i think. i have a documentation for the plugin i will post in this next days that will make it easier to understand, there is going to probably be a hotfix before the next bitshares-core release for a bug we already identified in the limit orders tracking. i will wait until that to get started.

pnomolos · 2018-03-20T20:54:35Z

@oxarbitrage Thanks!

es_objects plugin

4570387

abitmore reviewed Nov 24, 2017

View reviewed changes

oxarbitrage added 3 commits November 24, 2017 11:07

remove magic numbers and some commented includes

26c57fd

check for nullptr at casting

7b18456

remove non needed space_id from plugin

7b453c6

oxarbitrage mentioned this pull request Nov 28, 2017

Elasticsearch plugin #444

Merged

oxarbitrage added 2 commits November 30, 2017 19:14

add all account authorities

3545c22

add additional data to proposals

fe4e87b

pnomolos mentioned this pull request Jan 26, 2018

[6] View referrals in wallet bitshares/bitshares-ui#661

Closed

oxarbitrage mentioned this pull request Feb 9, 2018

add api calls to get non expired withdrawls #657

Closed

add limit order objects

fddf08d

oxarbitrage mentioned this pull request Feb 12, 2018

Feature: able to set an ID in limit_order_create operation #556

Open

oxarbitrage mentioned this pull request Feb 19, 2018

add cli wallet calls and unit tests for new withdraw permission apis #677

Open

4 tasks

pmconrad mentioned this pull request Feb 25, 2018

Post settlement price changes to ES #690

Open

add utility common elasticsearch calls

2bf302d

oxarbitrage mentioned this pull request Feb 25, 2018

Return total number of available assets #688

Closed

7 tasks

xeroc reviewed Mar 2, 2018

View reviewed changes

oxarbitrage added 4 commits March 5, 2018 12:07

simplify database call

207eb07

add startup ilog

109b19f

add bitasset support, other cleanup

b00767d

Merge branch 'develop' of https://github.com/bitshares/bitshares-core …

f855926

…into elasticsearch-extras

abitmore approved these changes Mar 19, 2018

View reviewed changes

abitmore added this to the 201803 Non-Consensus-Changing Release milestone Mar 19, 2018

change local variable name, change some default values, make block_ti…

1916803

…me a const

add metadata to indexes

1dc87f2

abitmore approved these changes Mar 20, 2018

View reviewed changes

oxarbitrage merged commit 51e6c79 into bitshares:develop Mar 20, 2018

startailcoon mentioned this pull request Apr 2, 2018

Find Market only shows OPEN assets bitshares/bitshares-ui#1320

Closed

vikramrajkumar mentioned this pull request Jan 18, 2017

Documentation of new market API #171

Closed

gladcow mentioned this pull request Dec 6, 2019

ElasticSearch Plugin to support for Peerplays peerplays-network/peerplays#161

Closed

16 tasks

es_objects plugin #500

es_objects plugin #500

Conversation

oxarbitrage commented Nov 23, 2017

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Nov 30, 2017

pnomolos commented Feb 25, 2018

oxarbitrage commented Feb 25, 2018

oxarbitrage commented Feb 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeroc commented Mar 2, 2018

oxarbitrage commented Mar 13, 2018

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Mar 20, 2018

abitmore commented Mar 20, 2018

oxarbitrage commented Mar 20, 2018 • edited Loading

abitmore commented Mar 20, 2018

abitmore left a comment

Choose a reason for hiding this comment

pnomolos commented Mar 20, 2018

oxarbitrage commented Mar 20, 2018

pnomolos commented Mar 20, 2018

oxarbitrage commented Mar 20, 2018 •

edited

Loading