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

elasticsearch history api #1682 #1725

Merged
merged 16 commits into from
Aug 14, 2019
Merged

Conversation

oxarbitrage
Copy link
Member

#1682

  • Added get_operation_by_id as requested at ES Plugin: Get operation by ID #1682 (comment) to elasticsearch plugin.

  • Added get_account_history to elasticsearch and use it when plugin is available to serve get_account_history api calls.

  • Test cases added are the same as history_api_tests/get_account_history_additional to make sure results are consistent.

Considerations:

  • Option elasticsearch-operation-string was added and is needed to serve the history api calls. The op_object field is modified to fit elasticsearch when indexing, we need the original object in a string to transmit it back on request. This will increase the size of the resources needed for ES but as an optional option users can use the string if they only want the node to serve history api calls or save the object for kibana/others. Can also store the 2 things if he haves the space.

Questions/TODO:

  • Implement get_account_history_operations - should be easy, it is almost the same query as get_account_history.
  • Implement get_relative_account_history - this is a bit trickier to do in ES - do we need it ?
  • Add get_operation_by_id as api call?

libraries/plugins/elasticsearch/elasticsearch_plugin.cpp Outdated Show resolved Hide resolved
libraries/app/api.cpp Outdated Show resolved Hide resolved
libraries/app/api.cpp Outdated Show resolved Hide resolved
libraries/app/api.cpp Outdated Show resolved Hide resolved
@abitmore abitmore added this to In development in Feature Release (3.1.0) via automation Apr 23, 2019
@abitmore abitmore added this to the 3.1.0 - Feature Release milestone Apr 23, 2019
@ryanRfox ryanRfox added this to In development in Feature Release (3.2.0) via automation Apr 23, 2019
@ryanRfox ryanRfox removed this from In development in Feature Release (3.1.0) Apr 23, 2019
@pmconrad pmconrad added this to In development in Feature Release (3.3.0) via automation Jun 19, 2019
@pmconrad pmconrad removed this from In development in Feature Release (3.2.0) Jun 19, 2019
if(_app.is_plugin_enabled("elasticsearch")) {
auto es = _app.get_plugin<elasticsearch::elasticsearch_plugin>("elasticsearch");
auto _thread = std::make_shared<fc::thread>("elasticsearch");
return _thread->async([&](){ return es->get_account_history(account, stop, limit, start); },
Copy link
Contributor

Choose a reason for hiding this comment

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

Please capture only what's necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, don't use this if mode is only_save.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually work as intended? That would be cool.
We should load-test this before putting it into production.

libraries/app/application.cpp Outdated Show resolved Hide resolved
libraries/plugins/elasticsearch/elasticsearch_plugin.cpp Outdated Show resolved Hide resolved
{
"range": {
"block_data.block_time": {
"gte": "now-20y",
Copy link
Contributor

Choose a reason for hiding this comment

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

Put a TODO in your calendar for the year 2034: bump this. ;-)

Can't you leave out this part of the query?

Choose a reason for hiding this comment

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

I agree that the range could be omitted here.

Also I am wondering why you do query_string and not

"query": {
        "match" : {
            "account_history.operation_id" : "operation_id_string"
        }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

my apologies, this was not a production ready query but just a proof of concept probably pasted from kibana. i had changed this at 1f6ac14

Copy link
Member Author

Choose a reason for hiding this comment

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

for the second query i removed the range but query_string needs to stay as we are making a lucene query there and not just matching a field. 63f7aff

libraries/plugins/elasticsearch/elasticsearch_plugin.cpp Outdated Show resolved Hide resolved
@@ -59,6 +58,8 @@ class elasticsearch_plugin_impl
std::string _elasticsearch_index_prefix = "bitshares-";
bool _elasticsearch_operation_object = false;
uint32_t _elasticsearch_start_es_after_block = 0;
bool _elasticsearch_operation_string = true;
std::string _elasticsearch_mode = "only_save";
Copy link
Contributor

Choose a reason for hiding this comment

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

For more efficient comparison, make this an enum type.

Copy link
Member Author

@oxarbitrage oxarbitrage Jul 31, 2019

Choose a reason for hiding this comment

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

It is a bit tricky to do this because the boost::program_options will not accept the enum but basically just a string, number or boolean.
I picked the number option to do a static cast to the enum by checking the upper boundary to avoid inserting invalid options.

Implemented at de76301

Copy link
Member Author

Choose a reason for hiding this comment

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

changed the exception introduced here to graphene::chain::plugin_exception in the context of 5838a38


if(my->_elasticsearch_mode != "only_query") {
if (my->_elasticsearch_mode == "all")
my->_elasticsearch_operation_string = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Silently overriding this will cause confusion.
IMO add log output if user has configured this to false. Or perhaps fail to make this very explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, i am now not allowing the mode to be all if operation_string is false so no silent changes are made.

5838a38

("elasticsearch-mode", boost::program_options::value<std::string>(),
"Mode of operation: only_save, only_query, all(only_save)")
("elasticsearch-mode", boost::program_options::value<uint16_t>(),
"Mode of operation: only_save(0), only_query(1), all(2) - Default: 0")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use values 1,2,3 so the option behaves like a bitset. But that's just me.

libraries/app/api.cpp Outdated Show resolved Hide resolved
@jmjatlanta jmjatlanta moved this from In development to In testing in Feature Release (3.3.0) Aug 7, 2019
@oxarbitrage oxarbitrage merged commit cea7b74 into bitshares:develop Aug 14, 2019
Feature Release (3.3.0) automation moved this from In testing to Done Aug 14, 2019
@oxarbitrage oxarbitrage mentioned this pull request Aug 14, 2019
17 tasks
"query": {
"match":
{
"account_history.operation_id": )" + operation_id_string + R"("
Copy link
Member

@abitmore abitmore Dec 29, 2021

Choose a reason for hiding this comment

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

A " is missing before )" + operation_id_string.

It seems this code is not used anywhere though.

@sschiessl-bcp I found that you were in the conversation, do you remember anything about this?

Update: fixed in #2573 via 8cf4144.

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

5 participants