[#12048] V9 migration and verification script optimization #12896
Conversation
|
Hi @FergusMok, thank you for your interest in contributing to TEAMMATES!
Please address the above before we proceed to review your PR. |
| // 100 is the optimal batch size as there won't be too much time interval | ||
| // between read and save (if transaction is not used) | ||
| // cannot set number greater than 300 | ||
| // see | ||
| // https://stackoverflow.com/questions/41499505/objectify-queries-setting-limit-above-300-does-not-work |
There was a problem hiding this comment.
I've verified that this is no longer true. Running it on staging with 1000 does not lead to any crashes. However, it's slower on average compared to the original 100.
| @@ -110,6 +110,7 @@ public static void buildSessionFactory(String dbUrl, String username, String pas | |||
|
|
|||
| Configuration config = new Configuration() | |||
| .setProperty("hibernate.dialect", "org.hibernate.dialect.PostgreSQLDialect") | |||
| .setProperty("hibernate.jdbc.batch_size", "50") | |||
There was a problem hiding this comment.
I'm not sure if we want to commit this to master, as it's only for migration purposes. Maybe we could comment this line for master and put another comment on the migration script to remember to uncomment this line.
There was a problem hiding this comment.
Is there a way to change the config in the script? Otherwise I think we can put a comment in script and comment this line out on master.
There was a problem hiding this comment.
I don't think so. I've added the notes
FergusMok
changed the title
| HibernateUtil.beginTransaction(); | ||
| for (Map.Entry<String, Instant> entry : oldAccount.getReadNotifications().entrySet()) { | ||
| HibernateUtil.beginTransaction(); | ||
| UUID notificationId = UUID.fromString(entry.getKey()); | ||
| Notification newNotification = HibernateUtil.get(Notification.class, notificationId); | ||
| HibernateUtil.commitTransaction(); |
There was a problem hiding this comment.
Will this cause a problem if one read notification migration fail?
There was a problem hiding this comment.
If the migration does the early escape at if (newNotification == null) {? I think probably not, because it's outside of the entire loop
|
Running it on staging (Singapore for datastore and SQL) for 10000 AccountRequest migration and verification: Before: 294s |
|
Yep the notes looks good, thanks for the optimization! Now just need to fix the lint |
| List<T> sqlEntities = lookupSqlEntitiesByPageNumber(currPageNum); | ||
| long endTimeForSql = System.currentTimeMillis(); | ||
| log("Querying for SQL for page " + currPageNum + " took " |
There was a problem hiding this comment.
Do these timings (here and DataMigrationEntitiesBaseScriptSql.java) affect how readable the logged output is? If it is perhaps we can remove it from the PR or add a flag/ log option to only output on debug or something
There was a problem hiding this comment.
It's less verbose because I've adjusted batch size to be 1000 instead of 100. The log statements can also show us what to further optimize if it's still slow during migration.
…EAMMATES#12896) * Add changes * Add changes for migration * Revert the illegals * Linting * Add additional batching * Add notes * Fix linting errors --------- Co-authored-by: Zhang Ziqing <69516975+ziqing26@users.noreply.github.com>
Part of #12048
idinstead ofcreatedAtto take advantage of SQL's indexes.