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

handle changeListener inputs in transactions correctly #360

Open
mqus opened this issue Jun 14, 2020 · 2 comments · May be fixed by #537
Open

handle changeListener inputs in transactions correctly #360

mqus opened this issue Jun 14, 2020 · 2 comments · May be fixed by #537
Labels
feature Request and implementation of a feature (release drafter)

Comments

@mqus
Copy link
Collaborator

mqus commented Jun 14, 2020

This is something that would help us in handling the changes made by transactions and lessen the amount of updates that take place.

Currently, database changes made in @transaction functions immediately notify the changeListener and start to update the table in streams. This has the following problems:

  • The tables are updated, even if the transaction is not finished yet (and therefore no changes have been persisted yet)
  • If a transaction e.g. deletes some entries in a table and immediately inserts them again, two notifications are put inside the changeListener, adding unnecessary refresh cycles.
  • When a stream (e.g. on joins) has to listen on multiple entity names at once, even more unnecessary refreshes happen.
  • If the transaction fails, the streams will get updated even if nothing happened.

Because of the reasons above, I propose to change the type of the changeListener from Stream<String> to Stream<Set<String>> and put a proxy changeListener into the transaction generation (Line 32, see below), which buffers all changes and then submits them together once the transaction was completed successfully. This adds some overhead into transactions (for the proxy) and some smaller overhead for streams, which now have calculate an intersection, instead of a simple contains calculation for filtering relevant updates.

I want to hear your input first, @vitusortner and I think that I probably won't start to work on this soon. (I'm currently having more fun with the sqlparser 😉 )

https://github.com/vitusortner/floor/blob/18117ed397b5787905576aedad90e0f347931792/floor_generator/lib/writer/transaction_method_writer.dart#L27-L36

A small note: I was surprised to see that adding this to transactions is far easier than I imagined.

@mqus mqus added the feature Request and implementation of a feature (release drafter) label Jun 14, 2020
@mqus
Copy link
Collaborator Author

mqus commented Jun 14, 2020

Ah right, one more thing:

  • Is there a reason why the transaction currently only returns void? Afaict it should be able to return anything (wrapped inside a Future, obviously).

@mqus
Copy link
Collaborator Author

mqus commented Apr 18, 2021

Ah right, one more thing:

* Is there a reason why the transaction currently only returns void? Afaict it should be able to return anything (wrapped inside a Future, obviously).

was adressed in #381

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Request and implementation of a feature (release drafter)
Development

Successfully merging a pull request may close this issue.

1 participant