feat: Add BulkImport APIs and cron #211

anku255 · 2024-03-20T07:31:04Z

Summary of change

(A few sentences about this PR)

Related issues

Bulk User Import into SuperTokens supertokens-core#912

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

Changelog has been updated
pluginInterfaceSupported.json file has been updated (if needed)
Changes to the version if needed
- In build.gradle
Had installed and ran the pre-commit hook
If there are new dependencies that have been added in build.gradle, please make sure to add them in implementationDependencies.json.
Issue this PR against the latest non released version branch.
- To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
- If no such branch exists, then create one from the latest released branch.
When adding new recipes, ensure that its performance is being measured in the OneMillionUsersTest

src/main/java/io/supertokens/storage/postgresql/BulkImportProxyStorage.java

src/main/java/io/supertokens/storage/postgresql/queries/BulkImportQueries.java

CHANGELOG.md

sattvikc · 2024-04-22T06:21:15Z

build.gradle

-version = "7.0.0"
+version = "7.1.0"


Update branch with latest and update versions accordingly

src/main/java/io/supertokens/storage/postgresql/QueryExecutorTemplate.java

rishabhpoddar · 2024-05-05T09:05:29Z

src/main/java/io/supertokens/storage/postgresql/BulkImportProxyStorage.java

+        // We are overriding the loadConfig method to set the connection pool size
+        // to 1 to avoid creating many connections for the bulk import cronjob
+        configJson.addProperty("postgresql_connection_pool_size", 1);
+        Config.loadConfig(this, configJson, logLevels, tenantIdentifier);


better to just call super.loadConfig instead?

rishabhpoddar · 2024-05-05T09:10:41Z

src/main/java/io/supertokens/storage/postgresql/queries/GeneralQueries.java

+        {
+            String DROP_QUERY = "DROP INDEX IF EXISTS bulk_import_users_status_updated_at_index";
+            update(start, DROP_QUERY, NO_OP_SETTER);
+        }
+        {
+            String DROP_QUERY = "DROP INDEX IF EXISTS bulk_import_users_created_at_index";
+            update(start, DROP_QUERY, NO_OP_SETTER);
+        }
        {


you are creating 3 indexes, but dropping just 2

rishabhpoddar · 2024-05-05T09:12:18Z

src/main/java/io/supertokens/storage/postgresql/queries/BulkImportQueries.java

+        return "CREATE TABLE IF NOT EXISTS " + tableName + " ("
+                + "id CHAR(36),"
+                + "app_id VARCHAR(64) NOT NULL DEFAULT 'public',"
+                + "primary_user_id VARCHAR(64),"


why is this 64 chars? SuperTokens user id is 36 chats

Updated it to 36 chars.

rishabhpoddar · 2024-05-05T09:14:12Z

src/main/java/io/supertokens/storage/postgresql/queries/BulkImportQueries.java

+                + "CONSTRAINT " + Utils.getConstraintName(schema, tableName, "app_id", "fkey") + " "
+                + "FOREIGN KEY(app_id) "


i assume all the entries of all apps will be in the public storage? In that case, if an app is pointing to a different storage, that app's app_id won't be in the public storage's apps table. Have you tested this? Maybe im wrong.

rishabhpoddar · 2024-05-05T09:16:29Z

src/main/java/io/supertokens/storage/postgresql/queries/BulkImportQueries.java

+        update(start, queryBuilder.toString(), pst -> {
+            int parameterIndex = 1;
+            for (BulkImportUser user : users) {
+                pst.setString(parameterIndex++, user.id);


if there is duplicate user id here, will the whole query fail?

Yes, the whole query will fail.

rishabhpoddar · 2024-05-05T09:21:11Z

src/main/java/io/supertokens/storage/postgresql/Start.java

@@ -3071,4 +3097,83 @@ public int getDbActivityCount(String dbname) throws SQLException, StorageQueryEx
            return -1;
        });
    }
+
+    @Override


the functions below are all supposed to be called NOT on the bulkimportproxystorage instance right? In that case, please assert in these that the instance of this is not of type bulkimportproxystorage

rishabhpoddar · 2024-05-05T09:21:54Z

src/main/java/io/supertokens/storage/postgresql/queries/BulkImportQueries.java

+                // NOTE: On average, we take about 66 seconds to process 1000 users. If, for any reason, the bulk import users were marked as processing but couldn't be processed within 10 minutes, we'll attempt to process them again.
+                String selectQuery = "SELECT * FROM " + Config.getConfig(start).getBulkImportUsersTable()
+                        + " WHERE app_id = ?"
+                        + " AND (status = 'NEW' OR (status = 'PROCESSING' AND updated_at < (EXTRACT(EPOCH FROM CURRENT_TIMESTAMP) * 1000) -  10 * 60 * 1000))" /* 10 mins */


why is 10 mins a good value?

On average we take around 66 seconds to process 1000 users but we chose 10 mins wait time to give us ample amount of time before retrying the users in case the processing was delayed.

The processing can fail because of reasons like (DB failure), 10 mins gives some time for those services to recover before next retry.

rishabhpoddar · 2024-05-05T09:22:14Z

src/main/java/io/supertokens/storage/postgresql/queries/BulkImportQueries.java

+                String selectQuery = "SELECT * FROM " + Config.getConfig(start).getBulkImportUsersTable()
+                        + " WHERE app_id = ?"
+                        + " AND (status = 'NEW' OR (status = 'PROCESSING' AND updated_at < (EXTRACT(EPOCH FROM CURRENT_TIMESTAMP) * 1000) -  10 * 60 * 1000))" /* 10 mins */
+                        + " LIMIT ? FOR UPDATE SKIP LOCKED";


i think we had decided not to use SKIP LOCKED?

We had decided to use SKIP LOCKED and we tested that it was safe to do so. I have added comment explaining why we need this.

feat: Add BulkImport APIs and cron

1f5d327

rishabhpoddar requested changes Mar 20, 2024

View reviewed changes

anku255 added 2 commits March 21, 2024 16:17

fix: PR changes

cc2ffe1

fix: PR changes

38882d7

sattvikc requested changes Mar 29, 2024

View reviewed changes

anku255 added 6 commits March 29, 2024 19:23

fix: PR changes

6d4da2e

fix: PR changes

c39e8e7

fix: PR changes

5634523

fix: PR changes

3b0df86

fix: Update version and changelog

737db91

fix: PR changes

1375803

sattvikc requested changes Apr 17, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

sattvikc requested changes Apr 22, 2024

View reviewed changes

src/main/java/io/supertokens/storage/postgresql/QueryExecutorTemplate.java Outdated Show resolved Hide resolved

anku255 added 2 commits April 25, 2024 16:06

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

c6a9329

fix: PR changes

3ecd8ea

rishabhpoddar requested changes May 5, 2024

View reviewed changes

anku255 mentioned this pull request May 29, 2024

Bulk User Import into SuperTokens supertokens/supertokens-core#912

Open

6 tasks

fix: PR changes

ecb078b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add BulkImport APIs and cron #211

feat: Add BulkImport APIs and cron #211

anku255 commented Mar 20, 2024 •

edited

sattvikc Apr 22, 2024

anku255 Apr 29, 2024

rishabhpoddar May 5, 2024

anku255 May 29, 2024

rishabhpoddar May 5, 2024

anku255 May 29, 2024

rishabhpoddar May 5, 2024

anku255 May 29, 2024

rishabhpoddar May 5, 2024

rishabhpoddar May 5, 2024

anku255 May 29, 2024

rishabhpoddar May 5, 2024

rishabhpoddar May 5, 2024

anku255 May 29, 2024

rishabhpoddar May 5, 2024

anku255 May 29, 2024

		+ "CONSTRAINT " + Utils.getConstraintName(schema, tableName, "app_id", "fkey") + " "
		+ "FOREIGN KEY(app_id) "

feat: Add BulkImport APIs and cron #211

Are you sure you want to change the base?

feat: Add BulkImport APIs and cron #211

Conversation

anku255 commented Mar 20, 2024 • edited

Summary of change

Related issues

Test Plan

Documentation changes

Checklist for important updates

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

anku255 commented Mar 20, 2024 •

edited