-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#12048] Data migration: ensure consistency when script is stopped and resumed #13046
[#12048] Data migration: ensure consistency when script is stopped and resumed #13046
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, looks good to me, just very tiny nits
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Outdated
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Outdated
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Outdated
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Outdated
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Outdated
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on getting this out, some changes are required though
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Outdated
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Show resolved
Hide resolved
*/ | ||
private void deleteCourseCascade(Course oldCourse) { | ||
// TODO: Implement deleteCourseCascase | ||
log("delete course id: " + oldCourse.getUniqueId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a one-liner, could we do this in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think calling HibernateUtil.remove(course) can cascade delete other entities, maybe need to do something like in CourseLogic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, I was originally thinking of using Cascade on delete annotation but there may be performance issues with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think performance issues should be fine because deleting is expected to only happen rarely (error in validation, exception thrown, or halfway migrated)
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java
Show resolved
Hide resolved
src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitiesSql.java
Outdated
Show resolved
Hide resolved
…ammates into v9-course-migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thank you for your work on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, especially on the more detailed parts of the logging and the cascade on users
Part of #12048
Outline of Solution
casecadeDelete
method will be called to delete all related entities in the sql database and migration will restart from A.Edit:
CascadeType.REMOVE
is already present for most parts of the chain but not present for Course-FeedbackSession, Course-User. For easier implementation fordeleteCourseCascade
, addedcascade = CascadeType.REMOVE
annotation for the two relations inCourse.java
.