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

start_date and end_date filter on balance API doesn't work correctly if an exisiting balance snapshot is present #109

Open
amitex007 opened this issue Nov 15, 2023 · 7 comments

Comments

@amitex007
Copy link

amitex007 commented Nov 15, 2023

Hi Medici team,
It seems that the balance API doesn't return the correct values if there is a cached balance entry in balance_snapshots.
If the start_date and end_date are values before the last transaction Id in the snapshot, the balance API just returns the balance in the snapshot.
link

I suggest that we should also maintain a start_time and end_time in the balance snapshot and check if the range query can be satisfied by the snapshot. Else recalculate accordingly.
Happy to raise a PR for the same if we agree on a solution.
Or please suggest a work-around which doesn't turn the caching feature completely off.

Thanks

@koresar
Copy link
Collaborator

koresar commented Nov 16, 2023

Hello

Thanks for reporting! The idea behind caching the balances is that the key for the cache would be the mongodb query itself. However, looks like we have a bug. The getBestSnapshot() should take dates into account for sure.

The correct fix would be:

  • pass both dates to the getBestSnapshot
  • the getBestSnapshot should use them to calculate the cache key

We'd be happy with your contribution!

@soumitdas
Copy link

Hi team, wanted to take this up.

Fix suggested by @koresar will be easy to do, but then it will not be balanceSnapshot anymore, it will work as querySnapshot for date range queries. Basically same date range queries will be taken from snapshot.

Ideal solution could be [as suggested by @amitex007] to store query start_date and end_date in balanceSnapshot itself, then getBestSnapshot will return the snapshot that lies in the range (when present) and cover maximum duration. We also need to store firstTransactionId apart from lastTransactionId to only aggregate on the transactions outside snapshot date range.

Way forward could be:

  1. First implement the date range in cacheKey approach as a start to fix the bug at-least.
  2. Second could be implement the date range caching to make the date range queries faster, but this will require balance schema change. [Shouldn't be a problem since we are using MongoDB & query start_date and end_date will be optional]

@koresar what do you say?

@koresar
Copy link
Collaborator

koresar commented Dec 9, 2024

@soumitdas thank you for thinking this through. I greatly support everything you've said. The item 2 would be especially valuable for everyone.

But I am certainly not going to implement this feature. No time. :)

Please, implement!

@soumitdas
Copy link

Please, implement!

Now we are aligned on the approach, I will start the implementation. Please assign the issue to me.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 9, 2024

We should not start cookie licking ;). Also i dont see any other person wanting to implement your suggestion. So you should be safe.

I also thought about this approach, few yeara ago. We need to ensure that balances are invalidated when there are new transactions made in that duration. This is the actual issue which needs extra focus.

@koresar
Copy link
Collaborator

koresar commented Dec 9, 2024

We need to ensure that balances are invalidated when there are new transactions made in that duration.

What does "transactions made in that duration" means? Like,

  • the date range is 2024-12-01 - 2024-12-31
  • we have a cached balance,
  • today, 2024-12-09, someone adds more ledger entries -> we invalidate the balance.

Right?

@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 9, 2024

Yes. And especially edge cases with seconds etc..

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

No branches or pull requests

4 participants