-
-
Notifications
You must be signed in to change notification settings - Fork 10.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
Refactor the soft delete design #3866
Refactor the soft delete design #3866
Conversation
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/entity/Item.java
Outdated
Show resolved
Hide resolved
3c2d891
to
a89c02b
Compare
Codecov Report
@@ Coverage Diff @@
## master #3866 +/- ##
============================================
+ Coverage 52.54% 52.56% +0.01%
- Complexity 2633 2635 +2
============================================
Files 486 488 +2
Lines 15244 15253 +9
Branches 1577 1577
============================================
+ Hits 8010 8017 +7
- Misses 6678 6682 +4
+ Partials 556 554 -2
Continue to review full report at Codecov.
|
All checks have passed!!! Unbelievable! I almost gave up.😂😂😂 @nobodyiam |
Congratulations! This is a remarkable milestone achieved! |
As another module use this Class, we can't move it to the test package in if you insist, we have two options:
Cause only one class, I prefer to keep it in main package. |
Thanks for the explanation, I think it's ok to leave it in apollo-common module. |
03cf95f
to
c692f8f
Compare
c692f8f
to
d7d81a7
Compare
@nisiyong would you please also update the CHANGES.md? |
e49082c
to
a232c63
Compare
After completing the unique index DDL, this PR now is ready. Thanks @nobodyiam give me a lot of suggestions. |
I exported the demo site data(access code: yu77) and tried to verify the upgrade steps. However, I met 2 errors when applying the
Notice we use the
Besides the empty key items, there are also some dirty data existing for items:
|
scripts/sql/delta/v1_9_0-v1_10_0/apolloconfigdb-v1_9_0-v1_10_0-after.sql
Outdated
Show resolved
Hide resolved
scripts/sql/delta/v1_9_0-v1_10_0/apolloportaldb-v1_9_0-v1_10_0-after.sql
Outdated
Show resolved
Hide resolved
In this case, we can find another DML to repair the duplicate dirty data. If it is difficult to do, we can temporarily give up adding a unique index for this table, due to the application logic layer adatping this case.
Because empty key is reasonable, I think maybe the unique index should consider the |
Maybe we could write a program to fix these dirty data? Before the user applies the
Yeah, this sounds reasonable. |
@nobodyiam |
Post some SQLs to check unique data before upgrade. -- ApolloConfigDB data unique check
select `AppId`,`Secret`,count(*) from `AccessKey` where `IsDeleted`=0 group by `AppId`,`Secret` having count(*) > 1;
select `AppId`,count(*) from `App` where `IsDeleted`=0 group by `AppId` having count(*) > 1;
select `AppId`,`Name`, count(*) from `AppNamespace` where `IsDeleted`=0 group by `AppId`,`Name` having count(*) > 1;
select `AppId`,`Name`,count(*) from `Cluster` where `IsDeleted`=0 group by `AppId`,`Name` having count(*) > 1;
select `AppId`,`ClusterName`,`NamespaceName`,count(*) from `Namespace` where `IsDeleted`=0 group by `AppId`,`ClusterName`,`NamespaceName` having count(*) > 1;
select `NamespaceId`,count(*) from `NamespaceLock` where `IsDeleted`=0 group by `NamespaceId` having count(*) > 1;
select `ReleaseKey`,count(*) from `Release` where `IsDeleted`=0 group by `ReleaseKey` having count(*) > 1;
select `Key`,`Cluster`,count(*) from `ServerConfig` where `IsDeleted`=0 group by `Key`,`Cluster` having count(*) > 1;
-- ApolloPortalDB data unique check
select `AppId`,count(*) from `App` where `IsDeleted`=0 group by `AppId` having count(*) > 1;
select `AppId`,`Name`, count(*) from `AppNamespace` where `IsDeleted`=0 group by `AppId`,`Name` having count(*) > 1;
select `AppId`,count(*) from `Consumer` where `IsDeleted`=0 group by `AppId` having count(*) > 1;
select `ConsumerId`,`RoleId`,count(*) from `ConsumerRole` where `IsDeleted`=0 group by `ConsumerId`,`RoleId` having count(*) > 1;
select `Token`,count(*) from `ConsumerToken` where `IsDeleted`=0 group by `Token` having count(*) > 1;
select `UserId`,`AppId`,count(*) from `Favorite` where `IsDeleted`=0 group by `UserId`,`AppId` having count(*) > 1;
select `TargetId`,`PermissionType`,count(*) from `Permission` where `IsDeleted`=0 group by `TargetId`,`PermissionType` having count(*) > 1;
select `RoleName`,count(*) from `Role` where `IsDeleted`=0 group by `RoleName` having count(*) > 1;
select `RoleId`,`PermissionId`,count(*) from `RolePermission` where `IsDeleted`=0 group by `RoleId`,`PermissionId` having count(*) > 1;
select `Key`,count(*) from `ServerConfig` where `IsDeleted`=0 group by `Key` having count(*) > 1;
select `UserId`,`RoleId`,count(*) from `UserRole` where `IsDeleted`=0 group by `UserId`,`RoleId` having count(*) > 1;
select `Username`,count(*) from `Users` where `IsDeleted`=0 group by `Username` having count(*) > 1; |
I think adding a |
132fc43
to
81a3733
Compare
81a3733
to
123228a
Compare
We can do this, but where to trigger this logic when the user upgrades apollo. |
5376008
to
2690297
Compare
I have removed the unique index on tables |
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.
This looks great to me!
BTW, shall we also add some upgrade instructions for those flyway users in this pr's changelog? e.g. first migrate to 2.0.0 and then after services are deployed, migrate to 2.0.1? It looks like this could be achieved by mvn -N -Pportaldb -Dflyway.target=2.0.0 flyway:migrate
but I haven't verified it.
apollo-common/src/main/java/com/ctrip/framework/apollo/common/jpa/H2Function.java
Outdated
Show resolved
Hide resolved
.../main/java/com/ctrip/framework/apollo/common/jpa/SqlFunctionsMetadataBuilderContributor.java
Outdated
Show resolved
Hide resolved
I test the Also provide my local console output log for you, if you need it. ➜ apollo git:(refactor-soft-delete) mvn -N -Pportaldb -Dflyway.target=2.0.0 flyway:migrate
Running `/Users/nisiyong/CodeProjects/apollo/mvnw`...
[INFO] Scanning for projects...
[INFO]
[INFO] -----------------< com.ctrip.framework.apollo:apollo >------------------
[INFO] Building Apollo 2.0.0-SNAPSHOT
[INFO] --------------------------------[ pom ]---------------------------------
[INFO]
[INFO] --- flyway-maven-plugin:5.2.4:migrate (default-cli) @ apollo ---
[INFO] Flyway Community Edition 5.2.4 by Boxfuse
[INFO] Database: jdbc:mysql://localhost:3306 (MySQL 5.7)
[INFO] Creating schema `ApolloPortalDB` ...
[INFO] Creating Schema History table: `ApolloPortalDB`.`flyway_schema_history`
[INFO] Current version of schema `ApolloPortalDB`: null
[INFO] Migrating schema `ApolloPortalDB` to version 1.0.0 - initialization
[WARNING] DB: Changing sql mode 'NO_AUTO_CREATE_USER' is deprecated. It will be removed in a future release. (SQL State: HY000 - Error Code: 3090)
[WARNING] DB: Changing sql mode 'NO_AUTO_CREATE_USER' is deprecated. It will be removed in a future release. (SQL State: HY000 - Error Code: 3090)
[INFO] Migrating schema `ApolloPortalDB` to version 1.1.1 - extend appId
[INFO] Migrating schema `ApolloPortalDB` to version 1.1.2 - extend username
[INFO] Migrating schema `ApolloPortalDB` to version 1.1.3 - add preferred username
[INFO] Migrating schema `ApolloPortalDB` to version 1.1.4 - delegating-password-encoder
[INFO] Migrating schema `ApolloPortalDB` to version 1.1.5 - jdbc-session
[INFO] Migrating schema `ApolloPortalDB` to version 2.0.0 - add-column-deletedat
[INFO] Successfully applied 7 migrations to schema `ApolloPortalDB` (execution time 00:01.445s)
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 2.565 s
[INFO] Finished at: 2022-03-14T23:30:03+08:00
[INFO] ------------------------------------------------------------------------
➜ apollo git:(refactor-soft-delete) mvn -N -Pportaldb flyway:migrate
Running `/Users/nisiyong/CodeProjects/apollo/mvnw`...
[INFO] Scanning for projects...
[INFO]
[INFO] -----------------< com.ctrip.framework.apollo:apollo >------------------
[INFO] Building Apollo 2.0.0-SNAPSHOT
[INFO] --------------------------------[ pom ]---------------------------------
[INFO]
[INFO] --- flyway-maven-plugin:5.2.4:migrate (default-cli) @ apollo ---
[INFO] Flyway Community Edition 5.2.4 by Boxfuse
[INFO] Database: jdbc:mysql://localhost:3306 (MySQL 5.7)
[INFO] Successfully validated 9 migrations (execution time 00:00.077s)
[INFO] Current version of schema `ApolloPortalDB`: 2.0.0
[INFO] Migrating schema `ApolloPortalDB` to version 2.0.1 - add-unique-key
[INFO] Successfully applied 1 migration to schema `ApolloPortalDB` (execution time 00:00.255s)
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.455 s
[INFO] Finished at: 2022-03-14T23:30:16+08:00
[INFO] ------------------------------------------------------------------------
|
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.
This looks great to me!
As this is a major change, I think it's better to have another approval.
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
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 have reviewed all changes. I checked as follows:
- upgrade steps
- all table's unique key
- all delete method has updated deleteAt field
It's great and look good to me.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/po/Permission.java
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.
ready to merge
I just added some useful SQL scripts to the PR description for users who might need them when upgrading Apollo. Please take a look. |
@nisiyong looks great! |
Which issue(s) this PR fixes:
Fixes #3700
Brief changelog
These CHANGES need to modify MySQL schemas. Upgrade Apollo with steps as follow:
deletedAt
columnscripts/sql/delta/v190-v200/apolloconfigdb-v190-v200.sql
scripts/sql/delta/v190-v200/apolloportaldb-v190-v200.sql
scripts/sql/delta/v190-v200/apolloconfigdb-v190-v200-after.sql
scripts/sql/delta/v190-v200/apolloportaldb-v190-v200-after.sql
If you were using Flyway manage your database schemas, upgrade Apollo with steps as follow:
deletedAt
columnmvn -N -Pconfigdb -Dflyway.target=2.0.0 flyway:migrate
mvn -N -Pportaldb -Dflyway.target=2.0.0 flyway:migrate
mvn -N -Pconfigdb flyway:migrate
mvn -N -Pportaldb flyway:migrate
Useful SQL scripts (Optional)
Scripts to check whether there is any duplicate data
Before you add unique index, you could use the following SQLs to check whether there are duplicate data in your database.
Scripts to rollback the unique indices
The following SQLs could rollback the unqiue indices if you want to degrade your application, and if you want to upgrade later, your could execute
scripts/sql/delta/v190-v200/*-v190-v200-after.sql
again.Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean test
to make sure this pull request doesn't break anything.CHANGES
log.