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

TP-841 - Order and partition transactions into partitions: SEED_FILE, REVISION, UPDATE. #334

Merged

Conversation

stuaxo
Copy link
Contributor

@stuaxo stuaxo commented Jul 14, 2021

This PR introduces partitioning to Transactions.

Global ordering is achieved by ordering on partition, order.

Partitions are:

SEED_FILE: Transactions imported from the initial seed file.
REVISION: Transactions imported or updated later.
DRAFT: Transactions in a workbasket.

During seed file update transactions may transitions are in the SEED_FILE partition.
During ordinary operation transactions are created as DRAFT, then when the WorkBasket is approved the transactions are changed to UPDATE and the order is rewritten to start after the highest REVISION transaction.

@stuaxo stuaxo changed the title Add status to transaction so to differenciate between transactions in… TP-841 - Enforce transaction order on publishing workbasket Jul 14, 2021
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 2 times, most recently from 677bb6f to 6155fb4 Compare July 15, 2021 17:36
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2021

Codecov Report

Merging #334 (c0b6a9c) into master (ef6dc60) will increase coverage by 0.02%.
The diff coverage is 92.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   91.26%   91.29%   +0.02%     
==========================================
  Files         276      276              
  Lines       16322    16572     +250     
  Branches     1326     1348      +22     
==========================================
+ Hits        14897    15129     +232     
- Misses       1193     1206      +13     
- Partials      232      237       +5     
Impacted Files Coverage Δ
additional_codes/models.py 95.23% <ø> (ø)
common/business_rules.py 97.69% <ø> (ø)
common/tests/test_serializers.py 100.00% <ø> (ø)
exporter/tests/test_exporter_tasks.py 100.00% <ø> (ø)
importer/forms.py 78.12% <50.00%> (+0.70%) ⬆️
importer/management/commands/run_import_batch.py 63.15% <66.66%> (+3.15%) ⬆️
exporter/sqlite/tasks.py 92.30% <75.00%> (-7.70%) ⬇️
workbaskets/models.py 88.60% <81.25%> (-5.40%) ⬇️
importer/tasks.py 66.66% <85.71%> (+0.51%) ⬆️
common/models/transactions.py 86.73% <88.46%> (+2.11%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef6dc60...c0b6a9c. Read the comment docs.

@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 2 times, most recently from 890e5eb to 445ffb0 Compare July 16, 2021 00:22
Copy link
Contributor

@simonwo simonwo left a comment

Choose a reason for hiding this comment

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

I like where this is going – will need some tests ofc.

To what extent should we be using the DRAFT/FINALISED flag elsewhere? Would it be more efficient to use it in TrackedModelQuerySet.approved_up_to_transaction(…) for example, instead of referring to the workbasket status?

common/models/transactions.py Outdated Show resolved Hide resolved
common/models/transactions.py Outdated Show resolved Hide resolved
common/models/transactions.py Outdated Show resolved Hide resolved
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 5 times, most recently from 9a3b71c to 3ca20d2 Compare July 29, 2021 22:30
common/validators.py Outdated Show resolved Hide resolved
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 2 times, most recently from aa89ab8 to cf49c05 Compare August 17, 2021 16:56
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 2 times, most recently from 1b57679 to 82eebef Compare August 31, 2021 10:36
@stuaxo stuaxo changed the title TP-841 - Enforce transaction order on publishing workbasket TP-841 - Order and partition transactions into partitions: SEED_FILE, DRAFT, UPDATE. Sep 1, 2021
@simonwo
Copy link
Contributor

simonwo commented Sep 2, 2021

I'm worried that UPDATE already has another use in our system as one of UPDATE, CREATE and DELETE update types. Is there another word?

Maybe REVISION?

@stuaxo stuaxo changed the title TP-841 - Order and partition transactions into partitions: SEED_FILE, DRAFT, UPDATE. TP-841 - Order and partition transactions into partitions: SEED_FILE, REVISION, UPDATE. Sep 6, 2021
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 3 times, most recently from 33b5845 to 4ed57ce Compare September 14, 2021 08:58
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 2 times, most recently from adeba68 to 8ecd1a4 Compare September 14, 2021 11:16
@stuaxo
Copy link
Contributor Author

stuaxo commented Sep 14, 2021

@simonwo one (non essential) thing I wanted to get working on this, was using RowNumber in the update(...) statement that corrects the order this would mean we could write transactions without any gaps, in the end I couldn't get this working, probably/hopefully missing something obvious there.

I guess gaps shouldn't be a problem - (though it opens a question about deletions: can transactions in a draft workbasket be deleted (probably) - and in that case should we consider removing them before making putting them in the REVISION partition ?

@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 2 times, most recently from 49e3766 to d722cb5 Compare September 16, 2021 07:32
common/models/records.py Outdated Show resolved Hide resolved
common/models/records.py Outdated Show resolved Hide resolved
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 6 times, most recently from 1bc0073 to 01f3486 Compare October 21, 2021 15:49
@stuaxo
Copy link
Contributor Author

stuaxo commented Oct 22, 2021

I like where this is going – will need some tests ofc.

To what extent should we be using the DRAFT/FINALISED flag elsewhere? Would it be more efficient to use it in TrackedModelQuerySet.approved_up_to_transaction(…) for example, instead of referring to the workbasket status?

Yes, probably everywhere - I think this should try and land and we can move everything else over that hasn't been as future PRs.

@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 10 times, most recently from b6f8c06 to 5f7b03e Compare October 26, 2021 09:45
@stuaxo stuaxo requested a review from simonwo October 26, 2021 10:08
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch 7 times, most recently from cd5bb59 to c0b6a9c Compare November 1, 2021 16:06
… updated afterwards (a revision) or are in a workbasket.

This allows these all to have seperate orders.

To allow a global order partion, order can be used.

To enable moving away from hardcoding that the first workbasket contains transactions, this PR introduces transactions schemas,
these can be set in settings and on import, with "seed_first" corresponding to the current implementation, "seed" and "revision"
create the corresponding transactions.

Data migrations are added to infer the value of the field.

Tests to verify transaction order are introduced, which in turn means making changes to factories so that transaction creation
and order more closely matches real world operation.

Tests found a pre-existingissue that can occur when certain records are placed into a transaction with each other and end up
out-of-order, this is pushed out to future work.
@stuaxo stuaxo force-pushed the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch from c0b6a9c to c98a0ba Compare November 1, 2021 16:24
@stuaxo stuaxo merged commit a3fb578 into master Nov 1, 2021
@stuaxo stuaxo deleted the TP-841-ensure-transaction-ids-are-contiguous-on-publishing branch November 1, 2021 16: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
3 participants