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

[WIP] Split trade statistics between recent data and historical data #4405

Closed

Conversation

chimp1984
Copy link
Contributor

First part: Add pruning support for trade statistics:

We add an interface for prunable PersistableNetworkPayloads so we can
handle it generically in the p2p module. We check at startup if our
persisted data has old elements and remove those. Data we receive from
the seed node at startup will get checked as well for old objects as
well data we receive from the network.

Next steps: Add historical data handling

We add an interface for prunable PersistableNetworkPayloads so we can
handle it generically in the p2p module. We check at startup if our
persisted data has old elements and remove those. Data we receive from
the seed node at startup will get checked as well for old objects as
well data we receive from the network.
@chimp1984 chimp1984 changed the title Split trade statistics between recent data and historical data [WIP] Split trade statistics between recent data and historical data Aug 8, 2020
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

This implementation throws away all old tradestats data and doesn't allow for any historical lookup. What is the plan for allowing historical data?

Instead of throwing data away during pruning it should probably be stored locally as HistoricalTradeStats which would work a bit like the split stores in #4233 except it's not a fixed block, but rather an append store but it can be excluded for all non historical data requests to seeds.

/**
* Interface for PersistableNetworkPayloads which can be pruned (e.g. old objects will be removed from data store).
*/
public interface PrunablePersistableNetworkPayload extends Payload {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to not let this inherit from PersistableNetworkPayload? That would make more sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good finding. Will change it.

@chimp1984
Copy link
Contributor Author

This implementation throws away all old tradestats data and doesn't allow for any historical lookup. What is the plan for allowing historical data?

Instead of throwing data away during pruning it should probably be stored locally as HistoricalTradeStats which would work a bit like the split stores in #4233 except it's not a fixed block, but rather an append store but it can be excluded for all non historical data requests to seeds.

Yes, the historical data part is not impl. yet... Not 100% sure about the details how it will be done.

Copy link
Contributor

@freimair freimair left a comment

Choose a reason for hiding this comment

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

NACK

  • pruning will not solve the problem: if bisq grows, we might need to prune after weeks or even days
  • your proposal seems like it would constantly shift objects from recent to historical datastore. Wouldn't that mean that the historical data store will grow as fast as the single data store does now?
  • adding code does not solve the underlying issue, it is just a risky fix that will be overpowered in months again
  • that adds even more complexity right now and down the road with only minimal functional gain
  • yet it leaves the existing code, which starts to become overpowered by the requirements and thus, desperately needs fixing, untouched and suffering

Requested Changes: come together in a call and talk about it, and yes, that will be a long one.

@freimair
Copy link
Contributor

freimair commented Sep 1, 2020

ah yes, and your plan sounds a lot like #4233, especially the splitting of data stores (only into 2 and keep updating the historical one)

I, however, do not see how you distinguish between recent and historical data, except by doing it by timestamp (which I believe you guys already agreed that this is NOT the way to go #4233 (comment))

@chimp1984
Copy link
Contributor Author

Closes as its WIP. Will continue to look into it over the next weeks.

@chimp1984 chimp1984 closed this Sep 1, 2020
@chimp1984 chimp1984 deleted the split-trade-statistic-data branch September 15, 2020 18:08
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.

3 participants