feat: multithreaded bulk import #1077

tamassoltesz · 2024-11-27T09:53:26Z

Summary of change

Core implementation of Bulk User Migration.

Related issues

Bulk User Import issue

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your
changes work. Bonus points for screenshots and videos!)

Documentation changes

(If relevant, please create a PR in our docs repo, or create a checklist here
highlighting the necessary changes)

Checklist for important updates

Remaining TODOs for this PR

Item1
Item2

sattvikc · 2024-12-09T09:55:26Z

config.yaml

+
+# (DIFFERENT_ACROSS_TENANTS | OPTIONAL | Default: number of available processor cores) int value. If specified,
+# the supertokens core will use the specified number of threads to complete the migration of users.
+# bulk_migration_parallelism:


shouldn't this be same for a given app? Does bulk import run for app or a tenant?

it runs for app, so yeah, it should be the same for the tenants. My bad, just followed the instructions without thinking it through.

sattvikc · 2024-12-09T09:55:44Z

devConfig.yaml

@@ -170,3 +170,8 @@ disable_telemetry: true

 # (Optional | Default: null) string value. The encryption key used for saving OAuth client secret on the database.
 # oauth_client_secret_encryption_key:
+
+# (DIFFERENT_ACROSS_TENANTS | OPTIONAL | Default: number of available processor cores) int value. If specified,


refer comment from config.yaml

sattvikc · 2024-12-09T10:01:51Z

src/main/java/io/supertokens/bulkimport/BulkImport.java

+    // Number of users to process in a single batch of ProcessBulkImportUsers Cron Job
+    public static final int PROCESS_USERS_BATCH_SIZE = 10000;
+    // Time interval in seconds between two consecutive runs of ProcessBulkImportUsers Cron Job
+    public static final int PROCESS_USERS_INTERVAL_SECONDS = 1;


Does it have to be this frequent?

yes. We want to squeeze out as much performance as we can. Setting it to 1 sec means that the new round of batch import processing is going to happen 1 seconds after the current round finished. This is the reason why the start/stop cronjob api was done: to not waste the resources when we don't need to import anything.

sattvikc · 2024-12-09T10:12:51Z

src/main/java/io/supertokens/bulkimport/BulkImportBackgroundJobManager.java

+
+public class BulkImportBackgroundJobManager {
+
+    public static BULK_IMPORT_BACKGROUND_PROCESS_STATUS startBackgroundJob(Main main, Integer batchSize) throws TenantOrAppNotFoundException {


does calling this twice create 2 cron jobs?

sattvikc · 2024-12-09T10:15:31Z

src/main/java/io/supertokens/bulkimport/BulkImportBackgroundJobManager.java

+    }
+
+    public static BULK_IMPORT_BACKGROUND_PROCESS_STATUS stopBackgroundJob(Main main) throws TenantOrAppNotFoundException {
+        ProcessBulkImportUsers processBulkImportUsersCron = (ProcessBulkImportUsers) main.getResourceDistributor().getResource(new TenantIdentifier(null, null, null), ProcessBulkImportUsers.RESOURCE_KEY);


what happens if the cron was already stopped and removed?

add test if possible

nothing. added a test for that

sattvikc · 2024-12-09T10:18:57Z

src/main/java/io/supertokens/config/CoreConfig.java

@@ -772,6 +782,10 @@ void normalizeAndValidate(Main main, boolean includeConfigFilePath) throws Inval
            }
        }

+        if (bulk_migration_parallelism < 1) {


do we also need an upper limit? what are the implications?

theoretically speaking the upper end is limitless, purely depending on the hardware. Of course, if this is set to an incredibly high number without the supporting hardware architecture beneath it, the JVM can crash when actually trying to bulk import. (The number of threads that are defined here are only instantiated when a bulk import batch is loaded for processing)

sattvikc · 2024-12-09T10:20:07Z

src/main/java/io/supertokens/cronjobs/Cronjobs.java

@@ -100,6 +100,17 @@ public static void addCronjob(Main main, CronTask task) {
        }
    }

+    // TODO test for this


sattvikc · 2024-12-09T10:26:33Z

src/main/java/io/supertokens/multitenancy/Multitenancy.java

@@ -228,7 +228,7 @@ public static boolean addNewOrUpdateAppOrTenant(Main main, TenantConfig newTenan
    }


-    public static boolean addNewOrUpdateAppOrTenant(Main main, TenantConfig newTenant,
+    public synchronized static boolean addNewOrUpdateAppOrTenant(Main main, TenantConfig newTenant,


why is this needed? I think we already take a lock for updating tenants.

hm, I am not sure what happened here. This is not even used in the bulk import..

removed the synchronized keyword

sattvikc · 2024-12-09T11:15:25Z

src/test/java/io/supertokens/test/bulkimport/apis/BulkImportBackgroundJobManagerAPITest.java

+        return HttpRequestForTesting.sendGETRequest(main, "", BACKGROUNDJOB_MANAGER_ENDPOINT, null,
+                1000, 1000, null, Utils.getCdiVersionStringLatestForTests(), null);
+    }
+


What's the behaviour if the job is stopped when an import is happening? can we test that as well?

Processing is stopped. Added a new test for that.

sattvikc · 2024-12-09T11:22:27Z

src/main/java/io/supertokens/webserver/api/bulkimport/BulkImportAPI.java

+
+        if (limit != null) {
+            if (limit > BulkImport.GET_USERS_PAGINATION_MAX_LIMIT) {
+                throw new ServletException(


Are u aware of typical post body size if say we are adding 10000 users? it would be a useful information to be able to set max post body size in the nginx for example.

well, 10k randomized users with multiple LMs and so on, are around 7.5MB (written to a json file). I don't know if that counts as typical or not.

sattvikc · 2024-12-09T11:23:00Z

src/main/java/io/supertokens/webserver/api/bulkimport/BulkImportBackgroundJobManagerAPI.java

+        return "/bulk-import/backgroundjob";
+    }
+
+    //TODO what's stopping two independent calls interfering with each-other?


yeah, but I don't have an answer for this. I can remove the comment but I don't have a solution for the written problem, apart from the clients who are orchestrating the imports should be aware. So, should I remove the comment? Do you have a better idea for solving this?

sattvikc · 2024-12-09T11:24:21Z

src/main/java/io/supertokens/webserver/api/bulkimport/CountBulkImportUsersAPI.java

+
+public class CountBulkImportUsersAPI extends WebserverAPI {
+  public CountBulkImportUsersAPI(Main main) {
+    super(main, "");


we could have an rid for bulk import. you could add it to all the bulk import APIs

sattvikc · 2024-12-09T11:25:54Z

src/main/java/io/supertokens/webserver/api/bulkimport/ImportUserAPI.java

+            super.sendJsonResponse(200, result, resp);
+        } catch (StorageQueryException e) {
+            JsonArray errors = new JsonArray();
+            if(e.getCause() instanceof BulkImportBatchInsertException){


this exception should be caught and thrown in BulkImport.importUser

sattvikc · 2024-12-09T11:27:53Z

src/main/java/io/supertokens/utils/Utils.java

+    public static boolean isAccountLinkingEnabled(Main main, AppIdentifier appIdentifier) throws StorageQueryException, TenantOrAppNotFoundException {
+        return Arrays.stream(FeatureFlag.getInstance(main, appIdentifier).getEnabledFeatures())
+                .anyMatch(t -> t == EE_FEATURES.ACCOUNT_LINKING || t == EE_FEATURES.MFA);
+    }


not sure why this change appears here

Because this was moved from AuthRecipe.java to the Utils.java as it was used in BulkImportUserUtils too, but with code duplication. I removed the code duplication and extracted this to a place where both participants can reach and doesn't go against the dependencies between the code layers.

anku255 and others added 30 commits March 20, 2024 12:58

feat: Add BulkImport APIs and cron

0ea0211

chore: update pull request template

f86211a

fix: Use the correct tenant config to create the proxy storage

05dc4a1

fix: PR changes

5b4658a

fix: PR changes

d721cd5

fix: PR changes

769cbb6

fix: PR changes

f443a8c

fix: PR changes

73b75a8

fix: PR changes

6d59616

fix: Update version

b6c63f4

fix: PR changes

173e7fc

fix: PR changes

4f6ab13

Merge branch 'feat/bulk-import-base' into feat/bulk-import-1

7f8ce62

fix: Rename DeleteBulkImportUser API path

fcdfb54

fix: disable bulk import for in-memory db

ddfaa1c

fix: a bug with createTotpDevices

c7ae1e4

fix: PR changes

d24bebf

feat: Add an api to import user in sync

09c1b85

feat: Add an api to get count of bulk import users

119ca1d

fix: PR changes

79a1e67

fix: Add error codes and plainTextPassword import

77cb57c

fix: PR changes

78979f4

feat: multithreaded bulk import

0d70735

fix: changelog update

b78541e

fix: add new test

0c0dea8

fix: fixing unreliable mutithreaded bulk import with mysql

67c42e9

chore: merging master to feature branch

72706f9

fix: review fixes

807f617

fix: fixing failing tests

f7cc349

feat: bulkimport flow tests

1ec5dde

tamassoltesz added 13 commits October 12, 2024 08:58

feat: bulk import cron starter api

2c14844

fix: tweaking params for faster import

d0d13f0

fix: tests

1d076b5

checkpoint

8070d45

fix: remove vacuuming

d2ec5cb

chore: merging master to feature branch

8ff6eea

fix: minor tweaks

16eda70

feat: bulk inserting the bulk migration data

7aaf422

fix: fast as a lightning

d33eb5e

fix: restoring lost method

a41f951

fix: reworked error handling to comform previous approach with messages

96f6e0a

fix: fixing tests

21e91fc

fix: fixing failing tests, changing version

4fe694a

tamassoltesz requested review from sattvikc and porcellus November 27, 2024 09:53

chore: update changelog

535d00b

tamassoltesz changed the title ~~Feat: multithreaded bulk import~~ feat: multithreaded bulk import Nov 27, 2024

tamassoltesz marked this pull request as ready for review November 27, 2024 11:02

tamassoltesz mentioned this pull request Nov 27, 2024

Bulk User Import into SuperTokens #912

Open

9 tasks

fix: fixing issues and failing tests

22b7726

sattvikc requested changes Dec 9, 2024

View reviewed changes

fix: review changes

d6402ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: multithreaded bulk import #1077

feat: multithreaded bulk import #1077

tamassoltesz commented Nov 27, 2024 •

edited

Loading

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024 •

edited

Loading

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024

sattvikc Dec 9, 2024

tamassoltesz Dec 10, 2024


		public class BulkImportBackgroundJobManager {

		public static BULK_IMPORT_BACKGROUND_PROCESS_STATUS startBackgroundJob(Main main, Integer batchSize) throws TenantOrAppNotFoundException {

feat: multithreaded bulk import #1077

Are you sure you want to change the base?

feat: multithreaded bulk import #1077

Conversation

tamassoltesz commented Nov 27, 2024 • edited Loading

Summary of change

Related issues

Test Plan

Documentation changes

Checklist for important updates

Remaining TODOs for this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamassoltesz Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamassoltesz commented Nov 27, 2024 •

edited

Loading

tamassoltesz Dec 10, 2024 •

edited

Loading