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

add additional checks to adaptor for live chain sync #1396

Merged
merged 4 commits into from
Oct 24, 2018

Conversation

oxarbitrage
Copy link
Member

Elasticsearch plugin with adaptor included at #1356 start to fail at around block 24M due to error:

java.lang.IllegalArgumentException: Can't merge a non object mapping [operation_history.op_object.policy] with an object mapping [operation_history.op_object.policy]

This pull attempts to fix it, testing now. Will check the full logs of all ES looking for errors until sync.

@abitmore abitmore added this to the 201810 - Feature Release milestone Oct 21, 2018
@oxarbitrage oxarbitrage changed the title add policy to adaptor add additional checks to adaptor for live chain sync Oct 21, 2018
@abitmore
Copy link
Member

Thanks for your hard work. Seeing you adding those I think it's better if you find all related fields and add them all. For example, active_special_authority comes from account options and I'm sure there are other similar fields there E.G. owner_special_authority, policy comes from vesting related operations and there may be related fields, and etc. Wish it helps.

@pmconrad
Copy link
Contributor

IMO converting to a string is a bad fix.
What do the existing fields look like?

@oxarbitrage
Copy link
Member Author

Ill try to find them all by following the ES logs. Service drops a failure each time one of this issues occur. Then we can try to do something better for some of them.

@oxarbitrage
Copy link
Member Author

Currently at block 21M and no mapping errors in the elasticsearch logs with the last commit.

@oxarbitrage
Copy link
Member Author

My server went down for maintenance overnight so i had to start over this morning. Currently in 21M blocks again and no errors.

@ryanRfox
Copy link
Contributor

My node is now fully synced based on (most) of these changes. I did not have the two owner_special_authority entries in my build. However, it log these as [DEBUG] entries in the elasticsearch.log file (only two entries) and continued to function. I feel this PR should be merged.

Copy link
Member

@cogutvalera cogutvalera left a comment

Choose a reason for hiding this comment

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

just as an idea about next code here

if (o.find("proposed_ops") != o.end())
{
o["proposed_ops"] = fc::json::to_string(o["proposed_ops"]);
}
if (o.find("initializer") != o.end())
{
o["initializer"] = fc::json::to_string(o["initializer"]);
}
if (o.find("policy") != o.end())
{
o["policy"] = fc::json::to_string(o["policy"]);
}
if (o.find("predicates") != o.end())
{
o["predicates"] = fc::json::to_string(o["predicates"]);
}
if (o.find("active_special_authority") != o.end())
{
o["active_special_authority"] = fc::json::to_string(o["active_special_authority"]);
}
if (o.find("owner_special_authority") != o.end())
{
o["owner_special_authority"] = fc::json::to_string(o["owner_special_authority"]);
}
it looks very similar, just with different fields, perhaps would be better to iterate through collection of fields.

Thanks !

@oxarbitrage
Copy link
Member Author

@cogutvalera i agree. I didn't know they were going to be that much to make something better.
Still, i think we can postpone that change for the next normal release as it is hard to sync everything again and we are running out of time, i know that with the changes in this release the chain synchronize fully without any errors.

@pmconrad i found some hints about some of the new fields with problems.

the case of operation_history.op_object.extensions.active_special_authority and operation_history.op_object.extensions.owner_special_authority seems to be the format:

["1",{"asset":"1.3.743","num_top_holders":5}]

This is not supported by elasticsearch.

Researching the others.

@oxarbitrage
Copy link
Member Author

similar issue with policy, example:

["1",{"start_claim":"2018-01-25T17:59:28","vesting_seconds":31536000}]

@oxarbitrage
Copy link
Member Author

the predicates is a similar issue in the assert_operation:

[["0",{"account_id":"1.2.96397","name":"openledger-wallet"}]]

@oxarbitrage
Copy link
Member Author

I think this can be merged as it is and open a new issue for code style and a possible way to insert some of those problematic fields.

The information lost here is minimal and we know is fully working. I am interested on having a working version in master and testnet and continue the work in the develop.

Also to discuss, plugin need to be checked after new operations are added, could have an unsupported field.

@pmconrad
Copy link
Contributor

https://www.elastic.co/guide/en/elasticsearch/reference/current/array.html :

...however, all values in the array must be of the same datatype...

That sucks. I'm surprised this doesn't bite us more often, since all static_variants serialize like this IIRC.
Then again, static variant serialization sucks even more. :-/

Ok for now, but we should change this in the next release IMO. E. g.

[ tag_no, { object:data }] -> { _type: type_name, object:data }

@oxarbitrage oxarbitrage merged commit 9f6630d into bitshares:release Oct 24, 2018
@oxarbitrage oxarbitrage mentioned this pull request Oct 24, 2018
2 tasks
@abitmore
Copy link
Member

Ok for now, but we should change this in the next release IMO. E. g.
[ tag_no, { object:data }] -> { _type: type_name, object:data }

If you mean to change the overall jsonification, I disagree, because it may break all the clients. It's a big change so I'd rather live with the sucking one. If change ES only, then perhaps acceptable.

@pmconrad
Copy link
Contributor

I mean it sucks generally, but we should only adapt this for ES.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants