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

Prune/limit storage of rejected & failed swaps #548

Open
sangaman opened this issue Oct 1, 2018 · 5 comments
Open

Prune/limit storage of rejected & failed swaps #548

sangaman opened this issue Oct 1, 2018 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers P3 low priority performance Improving performance, speed, scalability

Comments

@sangaman
Copy link
Collaborator

sangaman commented Oct 1, 2018

Currently, we savea a SwapDeal from the maker side even when we reject the deal due to a reason like no route or no order availaibility. I think we should change this to only track swaps where the deal has at least been accepted first (whether it ultimately succeeds or not). The goal would be to prevent tracking useless data - in practice I could see it being perfectly normal for swaps to be frequently rejected and it could quickly become a lot of data. This is open for discussion since there may be reasons to want to store rejected swaps that I am not aware of.

@sangaman sangaman added the swaps label Oct 1, 2018
@offerm
Copy link
Contributor

offerm commented Oct 3, 2018

This is open for discussion since there may be reasons to want to store rejected swaps that I am not aware of.

Exactly. there may be reasons to want to store rejected swaps. This is why we should store them.

I could see it being perfectly normal for swaps to be frequently rejected

What can be a good reason for this?

As an exchange/admin, I would like to know about these failures, analyze them and reduce future failures.

Also, this sounds like an optimization and we should defer it to a later stage when we better understand the usage patterns and be sure that we need to optimize.

@sangaman
Copy link
Collaborator Author

sangaman commented Oct 3, 2018

I'm not talking about failures due to an error per se but rather swaps that were never attempted in the first place. In an environment where there is a lot of trading going on, I imagine it could be fairly common to get multiple swap requests for the same order in a short period of time, where we'd have to reject all but one. Although I believe with #495 swaps rejected due to an order on hold are not stored ,so we wouldn't need to change anything on that front.

The other case that's currently being stored that I'm thinking we might not want to store is when a swap fails due to not being able to communicate with local LND instances, either they are disabled or a getinfo call fails.

In these cases the errors would still be logged, just potentially not stored to the database.

@sangaman sangaman added the P3 low priority label Oct 3, 2018
@offerm
Copy link
Contributor

offerm commented Oct 3, 2018

I understand you.
Still, I think we should not remove this "potential noise" until it becomes "real noise". Let's see how things develop and keep in mind that we can always do this.
Doing it now does not give real value.
My 2 cents

@kilrau kilrau added this to the 1.0.0 milestone Oct 26, 2018
@kilrau
Copy link
Contributor

kilrau commented Oct 26, 2018

I think we can store more or less everything for now and at some point prune failed swaps after a certain time/amount. Implementing this is what this issue is about.

@kilrau kilrau changed the title Don't store rejected swaps Prune/limit storage of rejected & failed swaps Oct 26, 2018
@kilrau kilrau modified the milestones: 1.0.0-sprint.12, 1.0.0, post-1.0.0 Apr 11, 2019
@kilrau kilrau removed the P3 low priority label Jun 11, 2019
@kilrau kilrau removed this from the post-1.0.0 milestone Jun 11, 2019
@kilrau kilrau added P3 low priority enhancement New feature or request good first issue Good for newcomers performance Improving performance, speed, scalability and removed swaps labels Oct 9, 2019
@engwarrior
Copy link
Contributor

Pls provide estimate for this task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers P3 low priority performance Improving performance, speed, scalability
Projects
None yet
Development

No branches or pull requests

4 participants