Elasticsearch refactor #1122

oxarbitrage · 2018-07-06T18:51:45Z

The pull is work in progress for #1103

Plugin was refactored and cleaned making it easier to understand to others and easier to maintain, elasticsearch basic auth was added, new field operation_id_num for easy filtering was added, node will be stopped at any insert failure avoiding operation gap, etc.

Logs were removed as they take too much space and the proper way to test is take snapshots and compare with operations inserted.

By now, tests were made up to block 10 million getting the exact same results in elastic and in RAM. A test in current block number will be done, the bitshares-core team is in the progress to rent a machine for this as a full traditional node is needed.

More commits will be added into this pull request before any merge specially in the es_objects plugin side, not cleaned yet. Also new commits are expected to address all the issues in #1103

pmconrad · 2018-07-26T19:41:51Z

libraries/plugins/es_objects/es_objects.cpp

         elog("Error sending data to database");
+         //return false;


Either remove the commented-out return statement or add curly braces around these two lines

pmconrad · 2018-07-26T20:05:42Z

libraries/plugins/elasticsearch/elasticsearch_plugin.cpp

+      if(!my->update_account_histories(b))
+      {
+         edump(("Error populating ES database, exiting to avoid history gaps."));
+         throw;


Need to throw graphene::chain::plugin_exception if you want to re-apply the block.

pmconrad · 2018-07-26T20:19:20Z

libraries/plugins/elasticsearch/elasticsearch_plugin.cpp

+}
+
+void elasticsearch_plugin_impl::prepareBulk(const account_transaction_history_id_type& ath_id,
+                                            const std::string& bulk_line )


bulk_line is a member of the plugin_impl, no need to pass it in as a parameter

pmconrad

Can't test this because I don't have an ES cluster ready. Error handling looks good, except for the throw - see my comment. Since this is probably hard to add to the unit test suite, I'd suggest you run a replay, then shut down your ES while the node is still replaying, then start ES up again and see if the replay goes through.

Generally, I think the code still leaves a lot of room for optimization, but it looks much cleaner now. Good job.

My impression of the es_objects plugin is that it contains a lot of boilerplate code that can probably be written much more tersely using templates. It seems the map<object_id_type,*_struct> maps are always added to but never cleared - this doesn't look like a good idea IMO.

pmconrad · 2018-07-27T15:45:07Z

libraries/utilities/elasticsearch.cpp

@@ -152,9 +127,65 @@ std::vector<std::string> createBulk(std::string index_name, std::string data, st

   bulk.push_back("{ \"index\" : { \"_index\" : \""+index_name+"\", \"_type\" : \"data\" "+create_string+" } }");


Better don't create JSON strings like this. Create a variant_object instead and let fc::json serialize it.

pmconrad · 2018-07-27T15:48:00Z

You are copying a lot of stuff around, might be worthwhile to avoid this wherever possible.
Use move operator when you insert bulk_line into bulk_lines for example.

oxarbitrage · 2018-07-27T22:57:05Z

Can't test this because I don't have an ES cluster ready. Error handling looks good, except for the throw - see my comment. Since this is probably hard to add to the unit test suite, I'd suggest you run a replay, then shut down your ES while the node is still replaying, then start ES up again and see if the replay goes through.

Great suggestion. Had been doing some tests with this, now changed to plugin_exception here: 0fe975c

The good thing about this is that when doing a replay and ES gets down the plugin will keep trying until it gets up again. Doing some testing in the first index created by the plugin(bitshares-2015-10) when doing all straight the index end up with 161,004 records. By stopping the ES service and restarting several times the index ends up with the exact same number of ops.

Only difference is it seems it send some duplicates but this gets ignored by the database as the indexing is done by operation ID.

Will keep working in the other suggestions, very appreciated.

abitmore · 2018-07-27T22:40:46Z

libraries/plugins/elasticsearch/elasticsearch_plugin.cpp

   std::string trx_id = "";
-   if(!b.transactions.empty() && oho->trx_in_block < b.transactions.size()) {
+   if(!b.transactions.empty() && oho->trx_in_block < b.transactions.size())


!b.transactions.empty() && is not needed.

abitmore · 2018-07-27T23:00:24Z

libraries/plugins/elasticsearch/elasticsearch_plugin.cpp

+const account_statistics_object& elasticsearch_plugin_impl::getStatsObject(const account_id_type account_id)
+{
+   graphene::chain::database& db = database();
+   const auto &stats_obj = account_id(db).statistics(db);


Rebase to latest develop branch, use database::get_account_stats_by_owner() for better performance.

abitmore · 2018-07-28T19:55:46Z

libraries/plugins/elasticsearch/elasticsearch_plugin.cpp

+   fc::mutable_variant_object bulk_header;
+   bulk_header["_index"] = index_name;
+   bulk_header["_type"] = "data";
+   bulk_header["_id"] = fc::to_string(ath_id.space_id) + "." + fc::to_string(ath_id.type_id) + "." + ath_id.instance;


Why not use something like fc::to_string(ath) or even directly use ath?

yes, i don't like this line either. if i do fc::to_string(ath.id) it will add double " to the final string. if i do ath_id directly the compiler complains that cant convert it. looking for alternatives.

Makes sense. Thanks. Current code is fine.

oxarbitrage mentioned this pull request Jul 16, 2018

Elasticsearch: flush the pending ops after processing a block #1137

Closed

abitmore added this to In Development in Feature Release (201808) via automation Jul 21, 2018

abitmore added this to the 201807 - Next Non-Consensus-Changing Release milestone Jul 21, 2018

abitmore added this to To Do in External DB (Elastic Search) for Query via automation Jul 21, 2018

pmconrad reviewed Jul 26, 2018

View reviewed changes

pmconrad reviewed Jul 27, 2018

View reviewed changes

abitmore mentioned this pull request Jul 27, 2018

Compile warnings #1194

Closed

abitmore reviewed Jul 27, 2018

View reviewed changes

abitmore reviewed Jul 28, 2018

View reviewed changes

oxarbitrage force-pushed the elasticsearch_tests2 branch from 3597743 to 376fffc Compare July 29, 2018 16:31

oxarbitrage closed this Jul 29, 2018

oxarbitrage force-pushed the elasticsearch_tests2 branch from 376fffc to 57f7ee4 Compare July 29, 2018 16:40

Feature Release (201808) automation moved this from In Development to Done Jul 29, 2018

This was referenced Jul 29, 2018

Elasticsearch refactor #1201

Merged

Improve performance of elasticsearch plugin #1224

Closed

oxarbitrage mentioned this pull request Aug 22, 2018

refine es_objects plugin #1271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elasticsearch refactor #1122

Elasticsearch refactor #1122

oxarbitrage commented Jul 6, 2018 •

edited

pmconrad Jul 26, 2018

pmconrad Jul 26, 2018

pmconrad Jul 26, 2018

pmconrad left a comment

pmconrad Jul 27, 2018

pmconrad commented Jul 27, 2018

oxarbitrage commented Jul 27, 2018 •

edited

abitmore Jul 27, 2018

abitmore Jul 27, 2018

abitmore Jul 28, 2018

oxarbitrage Jul 28, 2018 •

edited

abitmore Jul 28, 2018

		@@ -152,9 +127,65 @@ std::vector<std::string> createBulk(std::string index_name, std::string data, st

		bulk.push_back("{ \"index\" : { \"_index\" : \""+index_name+"\", \"_type\" : \"data\" "+create_string+" } }");

Elasticsearch refactor #1122

Elasticsearch refactor #1122

Conversation

oxarbitrage commented Jul 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmconrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmconrad commented Jul 27, 2018

oxarbitrage commented Jul 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage Jul 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Jul 6, 2018 •

edited

oxarbitrage commented Jul 27, 2018 •

edited

oxarbitrage Jul 28, 2018 •

edited