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

Market history plugin improvements #478

Merged
merged 5 commits into from
Nov 20, 2017

Conversation

abitmore
Copy link
Member

PR for #472, #467, #454.

Introduces two new options to market_history_plugin:

  • max-order-his-records-per-market:
    Will only store this amount of matched orders for each market in order
    history for querying, or those meet the other option, which has more data.
    (default: 1000)
  • max-order-his-seconds-per-market:
    Will only store matched orders in last X seconds for each market in order
    history for querying, or those meet the other option, which has more data.
    (default: 259200 (3 days))

With these default settings, running current BitShares main net, RAM usage is
VIRT=4389536, RES=3.287g.

In comparison to earlier results mentioned in #472 (comment) (VIRT = 5982640, RES = 4.805g), the new changes made in this PR saved another 1.6GB of RAM, so in total saved more than 2GB of RAM.

With the new by_market_time index, #458 will be able to be addressed.

abitmore and others added 5 commits November 12, 2017 20:50
This commit introduces two new options to market_history_plugin:
* `max-order-his-records-per-market`:
  Will only store this amount of matched orders for each market in order
  history for querying, or those meet the other option, which has more data.
  (default: 1000)
* `max-order-his-seconds-per-market`:
  Will only store matched orders in last X seconds for each market in order
  history for querying, or those meet the other option, which has more data.
  (default: 259200 (3 days))
@abitmore abitmore mentioned this pull request Nov 14, 2017
@oxarbitrage oxarbitrage self-requested a review November 16, 2017 12:09
if( !o.is_maker )
return;

const auto max_history = _plugin.max_history();
if( max_history == 0 ) return;
Copy link
Member

Choose a reason for hiding this comment

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

do we need this check ? i started to test the options with a setup as:

programs/witness_node/witness_node --data-dir data/my-blockprod --rpc-endpoint "127.0.0.1:8090" --max-ops-per-account 1000 --partial-operations true --max-order-his-seconds-per-market 259200 --max-order-his-records-per-market 0 --history-per-size 0

to later realize that i had to change history-per-size to at least 1 to make it work.

Copy link
Member Author

@abitmore abitmore Nov 16, 2017

Choose a reason for hiding this comment

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

There are two sets of data being stored in the market history plugin: the detailed order matching history, and the statistics (the data in buckets). history-per-size == 0 means to not store statistics, but it should not prevent order matching history from being stored. The original code skips statistics data storing if history-per-size is set to 0, but it also skips order matching history storing, which I think is a bug. Now the new options controls how order matching history being stored.

Copy link
Member

Choose a reason for hiding this comment

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

ok, got it. thanks for explaining it.

@oxarbitrage
Copy link
Member

history-per-size now working and removing old buckets. i hope this change do not break current applications relying in a core functionality not working.

i am a bit confused with the max-order-his-records-per-market new option. i am starting a node as:

programs/witness_node/witness_node --data-dir data/my-blockprod --rpc-endpoint "127.0.0.1:8090" --max-ops-per-account 1000 --partial-operations true --max-order-his-seconds-per-market 0 --max-order-his-records-per-market 3 --history-per-size 1

expecting to always get 2 records from a call like:

curl --data '{"jsonrpc": "2.0", "params": ["history","get_market_history", ["1.3.0", "1.3.113", 900, "2015-10-15T00:00:00", "2015-10-20T00:00:00", 100]], "method": "call", "id": 10}' --silent http://localhost:8090/rpc | jq

however i sometimes get 2 results, others just 1 as the chain replays until it gets out of range and deletes the buckets correctly.

same happening with a setup as (using : --max-order-his-seconds-per-market)

programs/witness_node/witness_node --data-dir data/my-blockprod --rpc-endpoint "127.0.0.1:8090" --max-ops-per-account 1000 --partial-operations true --max-order-his-seconds-per-market 3600 --max-order-his-records-per-market 0 --history-per-size 1

and querying the 15 mins buckets as before with:

curl --data '{"jsonrpc": "2.0", "params": ["history","get_market_history", ["1.3.0", "1.3.113", 900, "2015-10-15T00:00:00", "2015-10-20T00:00:00", 100]], "method": "call", "id": 10}' --silent http://localhost:8090/rpc | jq

where i am expecting to always get 4 results but instead i sometimes get 1, others 2, others 4, etc as the chain go passing throw the date range.

i may be misunderstanding the functionality, can you pls clarify/check @abitmore ? thank you very much.

@abitmore
Copy link
Member Author

@oxarbitrage explained in the review. Please check get_trade_history API and get_fill_order_history API which will be affected by the new options.

@abitmore
Copy link
Member Author

In regards to the bucket data, in order to save space, it's designed to not create an empty bucket if no trade occurred during that period. That's a bit inconvenient for markets that have few activities, because if got an empty result with a query, it's unable to know the open price nor the latest price.

@oxarbitrage
Copy link
Member

In regards to the bucket data, in order to save space, it's designed to not create an empty bucket if no trade occurred during that period. That's a bit inconvenient for markets that have few activities, because if got an empty result with a query, it's unable to know the open price nor the latest price.

that makes sense and it is actually a good idea but i am pretty sure clients will prefer to have all the data with no gaps inget_market_history witch is generally used to build charts. if no trade activity was present in a time interval i think it is preferred to return a bucket with the last trade data and correct timestamp. we could handle that in the api call itself thought(in a separated issue).

let me know what do you think and lets include @svk31 as he may also have some valuable input from the UI side.

@abitmore
Copy link
Member Author

@oxarbitrage I believe bitshares-ui is well handling the gaps, for example, check https://wallet.bitshares.org/#/market/OPEN.STEEM_BTC

@svk31
Copy link
Contributor

svk31 commented Nov 17, 2017

Yea the GUI fills in the gaps, creating new buckets at the close price of the last bucket before the gap.

Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

got it, thanks guys, all looks good to me in this pull after provided clarifications.

@oxarbitrage
Copy link
Member

i think we should merge this one. what do you think @abitmore ?

@abitmore
Copy link
Member Author

Yes

@oxarbitrage oxarbitrage merged commit e8da8e0 into bitshares:develop Nov 20, 2017
@abitmore abitmore added this to the Next Non-Consensus-Changing Release - 201712 milestone Nov 28, 2017
@abitmore abitmore deleted the 472-market-his-size branch June 28, 2021 21:59
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.

4 participants