noobaa/core: Transaction related changes #7249

aspandey · 2023-03-29T11:35:52Z

This patch makes changes to send bulk queries for
multiple collections in one transaction for postgres DB. In case of failure of any query, all the bulk operations will be rollbacked.

unit test has not been added for this patch

aspandey · 2023-03-29T11:38:28Z

@dannyzaken

src/server/system_services/system_store.js

src/util/postgres_client.js

aspandey · 2023-03-29T11:57:36Z

Thanks @liranmauda for comments. This is work in progress. Will make these changes in next iteration.

liranmauda · 2023-03-29T12:06:09Z

Thanks @liranmauda for comments. This is work in progress. Will make these changes in next iteration.

Yes, noticed the draft after I reviewed 🙂

dannyzaken · 2023-04-02T14:12:45Z

src/server/system_services/system_store.js

-            if (res && !res.ok) {
-                dbg.error('got error on bulk execute', res.err);
-                throw new Error(res.err);
+        if (config.DB_TYPE === 'postgres') {


Maybe worth hiding the different implementations in the db clients. I suggest adding a function to DBClient interface in nb.d.ts, and then you can have a different implementation for postgres\mongo. Maybe something like execute_multiple_bulks().
This function should return a common bulk_results array that should be inspected the same for both DBs

Done. Good Idea.

romayalon · 2023-04-23T09:30:37Z

src/util/postgres_client.js

+    const queries = _.flatten(Object.values(bulk_per_collection).map(bulk => bulk.get_queries()));
+    const transaction = new PgTransaction(client);
+
+    let ok = false;


this is the same code as execute() of bulkOp class, I don't see a value in maintaining this code in 2 different functions, easily can move to a helper function

Makes sense. Done.

src/server/system_services/system_store.js

dannyzaken

@aspandey, I discussed with @romayalon, and we agreed this change is ok. We reviewed the make_changes calls, and it seems to behave better with this change.
We also agreed that it makes sense to refactor the common code in postgers_client

romayalon · 2023-04-25T10:31:12Z

Hi @aspandey,
As Danny said I was worried about some specific make_changes() calls that make multiple changes and the queries are not related to each other.
For example, something like -

await system_store.make_changes({
        update: {
            buckets: buckets_updates,
            accounts: accounts_updates,
            pools: pools_updates,
            namespace_resources: namespace_resources_updates
        },
   });

I just wanted to make sure we won't roll back queries for no reason because this is the side effect of making one transaction per single make_changes() call instead of per collection in a make_changes() call.

dannyzaken · 2023-04-27T09:18:22Z

src/test/unit_tests/test_system_store.js

+        try {
+            await system_store.make_changes({
+                insert: {
+                    systems: [{


For the second make_changes you should add a change\insert to different tables (pools, accounts, etc.). After the failure, make sure all changes were not applied.
of course, this is the case for Postgres only, so we should skip this step for Mongo

dannyzaken · 2023-04-27T09:19:14Z

src/server/system_services/system_store.js

@@ -757,6 +756,7 @@ class SystemStore extends EventEmitter {
            }
        }

+


Unnecessary change.

dannyzaken · 2023-04-27T09:30:02Z

src/util/postgres_client.js

@@ -368,6 +368,79 @@ class PgTransaction {

 }

+async function run_queries(transaction, queries) {


I suggest renaming to something like run_queries_in_transaction.
Also, maybe it should be a static method of PgTransaction

This patch makes changes to send bulk queries for multiple collections in one transaction for postgres DB. In case of failure of any query, all the bulk operations will be rollbacked. Signed-off-by: Ashish Pandey <aspandey@redhat.com>

github-actions · 2024-05-30T11:00:52Z

This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed.

pull-request-size bot added the size/L label Mar 29, 2023

aspandey requested a review from dannyzaken March 29, 2023 11:37

aspandey marked this pull request as draft March 29, 2023 11:37

liranmauda reviewed Mar 29, 2023

View reviewed changes

src/server/system_services/system_store.js Outdated Show resolved Hide resolved

src/server/system_services/system_store.js Outdated Show resolved Hide resolved

liranmauda reviewed Mar 29, 2023

View reviewed changes

src/server/system_services/system_store.js Outdated Show resolved Hide resolved

liranmauda reviewed Mar 29, 2023

View reviewed changes

src/util/postgres_client.js Outdated Show resolved Hide resolved

dannyzaken requested a review from guymguym April 2, 2023 14:02

dannyzaken reviewed Apr 2, 2023

View reviewed changes

dannyzaken requested review from a team and Neon-White and removed request for a team April 4, 2023 12:21

aspandey force-pushed the transaction branch from aaaad01 to 681ed33 Compare April 6, 2023 11:01

aspandey marked this pull request as ready for review April 6, 2023 11:02

aspandey force-pushed the transaction branch from 681ed33 to be0c584 Compare April 6, 2023 11:06

dannyzaken requested a review from romayalon April 19, 2023 06:57

romayalon reviewed Apr 23, 2023

View reviewed changes

aspandey force-pushed the transaction branch from be0c584 to 1886448 Compare April 25, 2023 09:11

dannyzaken reviewed Apr 25, 2023

View reviewed changes

aspandey force-pushed the transaction branch 3 times, most recently from 923ddb5 to d7f1560 Compare April 26, 2023 09:03

dannyzaken reviewed Apr 27, 2023

View reviewed changes

aspandey force-pushed the transaction branch 2 times, most recently from d3019bc to 434618e Compare May 3, 2023 17:54

aspandey force-pushed the transaction branch from 434618e to 2da1ea1 Compare May 5, 2023 08:16

noobaa/core: Transaction related changes

7ab58e0

This patch makes changes to send bulk queries for multiple collections in one transaction for postgres DB. In case of failure of any query, all the bulk operations will be rollbacked. Signed-off-by: Ashish Pandey <aspandey@redhat.com>

aspandey force-pushed the transaction branch from 2da1ea1 to 7ab58e0 Compare May 17, 2023 06:57

dannyzaken mentioned this pull request Dec 11, 2023

Added preStop hook to DB statefulset noobaa/noobaa-operator#1263

Merged

2 tasks

github-actions bot added the Stale label May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

noobaa/core: Transaction related changes #7249

noobaa/core: Transaction related changes #7249

aspandey commented Mar 29, 2023 •

edited

aspandey commented Mar 29, 2023

aspandey commented Mar 29, 2023

liranmauda commented Mar 29, 2023

dannyzaken Apr 2, 2023

aspandey Apr 6, 2023

romayalon Apr 23, 2023

aspandey Apr 25, 2023

dannyzaken left a comment

romayalon commented Apr 25, 2023 •

edited

dannyzaken Apr 27, 2023

dannyzaken Apr 27, 2023

dannyzaken Apr 27, 2023

github-actions bot commented May 30, 2024

		@@ -757,6 +756,7 @@ class SystemStore extends EventEmitter {
		}
		}

		@@ -368,6 +368,79 @@ class PgTransaction {

		}

		async function run_queries(transaction, queries) {

noobaa/core: Transaction related changes #7249

Are you sure you want to change the base?

noobaa/core: Transaction related changes #7249

Conversation

aspandey commented Mar 29, 2023 • edited

aspandey commented Mar 29, 2023

aspandey commented Mar 29, 2023

liranmauda commented Mar 29, 2023

dannyzaken Apr 2, 2023

Choose a reason for hiding this comment

aspandey Apr 6, 2023

Choose a reason for hiding this comment

romayalon Apr 23, 2023

Choose a reason for hiding this comment

aspandey Apr 25, 2023

Choose a reason for hiding this comment

dannyzaken left a comment

Choose a reason for hiding this comment

romayalon commented Apr 25, 2023 • edited

dannyzaken Apr 27, 2023

Choose a reason for hiding this comment

dannyzaken Apr 27, 2023

Choose a reason for hiding this comment

dannyzaken Apr 27, 2023

Choose a reason for hiding this comment

github-actions bot commented May 30, 2024

aspandey commented Mar 29, 2023 •

edited

romayalon commented Apr 25, 2023 •

edited