FINERACT-274: Not able to create the same datatable which was rejected by the maker checker before#5495
FINERACT-274: Not able to create the same datatable which was rejected by the maker checker before#5495edk12564 wants to merge 1 commit intoapache:developfrom
Conversation
edk12564
force-pushed
the
FINERACT-274
branch
2 times, most recently
from
e473b39 to
54ae254
Compare
adamsaghy
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
I dont think any of the changes are needed.
The attached integration tests are green without messing with transaction boundaries, neither having the "cleanup".
The maker-checker is fully rolled back, so no left over data. You can create datatable if it was rejected priorly.
Kindly asking you to double check!
Firstly, thank you adam for the review! I did as you said and double checked by running a few tests, both in MariaDB and Postgres. Can I ask what database you used to check the changes? I think the database matters because the issue should only affect MariaDB. To clarify, the behavior of the solution and tests with a Postgres database should be a "no-op". This is because of how MariaDB handles DDL statements like creating new tables. So the problem doesn't even occur when Postgres databases are used. I tried to replicate in a few circumstances and got the results below. You can also see the cleanup commented out in (without cleanup) and not commented out in (with cleanup). MariaDB (without cleanup): MariaDB (with cleanup): Postgres (without cleanup): As for the extra @transactional I added. This was just to ensure that the removal didn't happen without the rest of the items in rejectEntry(). But thinking about it now, I don't think this is necessary, since MariaDB will commit this DDL even if it is in @transactional, just like for the table creation. I will make another change without this in there! Please let me know if there's something I missed. I understand I am new and thus prone to making mistakes. |
edk12564
force-pushed
the
FINERACT-274
branch
from
54ae254 to
ee86f22
Compare
OMG mariadb and mysql... OMG.... :( |
😄 😄 I heard from the recent meeting, but is MariaDB and MySQL getting phased out slowly? I understand we want to be accomodative to different solutions, but I'm getting the sense this doesn't apply here? |
Well... not as quickly as I want.. so probably this year we will still supporting 3 db engines :( |
| private final CommandProcessingService processAndLogCommandService; | ||
| private final SchedulerJobRunnerReadService schedulerJobRunnerReadService; | ||
| private final ConfigurationDomainService configurationService; | ||
| private final DatatableRejectionCleanupService ifDatatableRejectionThenCleanup; |
There was a problem hiding this comment.
Lets rename it something more generic, like: CleanUpService and have it as an interface only. Lets inject it as a List, so multiple implementations are allowed to be used (even if there is just one for now). If the list is null, we can skip it. if not a foreach we can call all of them. The implementation classes responsibilities to decide whether there is something to be done or not.
There was a problem hiding this comment.
I'll get to work on this now!
There was a problem hiding this comment.
Just pushed my changes. Please let me know if it is good!
adamsaghy
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
Kindly see my recommendations!
edk12564
force-pushed
the
FINERACT-274
branch
2 times, most recently
from
06610d7 to
58ecf98
Compare
- added datatable cleanup service after a maker checker rejection - checks if datatable was created in rejected command source, and if so deletes it - added integration test to assess orphan table deletion - accounted for MariaDB and PostgreSQL - accounted for group, center, loan account, office, and savings account levels - utilized more generic interface with options for more extensibility
edk12564
force-pushed
the
FINERACT-274
branch
from
58ecf98 to
e95e498
Compare
3 participants
JIRA: FINERACT-274, FINERACT-672, FINERACT-673, FINERACT-674, FINERACT-675, FINERACT-676
Problem
According to FINERACT-274, when a new datatable was rejected in the maker checker process, it creates an orphaned table by mistake, forcing developers to delete the table manually in their database. I've gone ahead and fixed this issue by adding a datatable cleanup service for maker checker rejections. More on this below.
To reproduce the problem:
Changes
The problem was found to affect the group, center, loan account, office, and savings accounts levels.
After diagnosing the issue, I found that during data table creation events, the transaction in maker-checker gets rolled backed. However, it turns out that MariaDB does not add DDL (data table changes) queries to a transaction, meaning when the maker-checker system rolls everything back for checking, the DDL actually executes and makes the table anyways. This means that when the data table is rejected, the orphaned data table exists in the database. PostgreSQL includes DDL as part of the transaction boundary, meaning it is not an issue when PostgreSQL is used.
To fix the issue, I added a cleanup service after every maker-checker rejection that checks if the rejection was a data table rejection. If that was the case, it cleans up the data table in question. If not, it continues as normal. This structure allows us to keep as much original logic as possible, while working in both PostgreSQL and MariaDB use cases.
To test my changes, I also added an integration test that tests the process of data table creation, maker-checker events, and another data table creation to determine if the data table was properly removed. I do this over the group, center, loan account, office, and saving account levels to make sure they don't produce the bugs mentioned in the various tickets.
I tried to follow convention as much as possible, but I may have missed some things so feedback is very welcome!
Results
Tests all pass:
For MariaDB:
For PostgreSQL:
Reproducing the issue no longer results in a "data table already exists" error:
Demo video is too big for Github, but here is the list of tables after submitting the maker command. You can see the test table.
And here is the list of tables after rejecting with the checker user. You can see there is no test table.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.