Do data validation when undo. #1060

ujjboy · 2019-05-17T03:54:22Z

Ⅰ. Describe what this PR did

Add logic:
- compare current data in the database with destination image, if they are not equal, it means have dirty data,
- then compare current data in the database and the original image, if they are equal, also no need to execute undo
- else will throw an exception
Add some test case

Ⅱ. Does this pull request fix one issue?

Part of #1049

ujjboy · 2019-05-17T03:55:07Z

I will add more test case which uses h2 database later.

codecov-io · 2019-05-17T14:36:24Z

Codecov Report

Merging #1060 into develop will increase coverage by 0.49%.
The diff coverage is 81.66%.

@@              Coverage Diff              @@
##             develop    #1060      +/-   ##
=============================================
+ Coverage      39.44%   39.94%   +0.49%     
- Complexity      1138     1153      +15     
=============================================
  Files            222      222              
  Lines           8872     8929      +57     
  Branches        1143     1158      +15     
=============================================
+ Hits            3500     3567      +67     
+ Misses          4931     4914      -17     
- Partials         441      448       +7

Impacted Files	Coverage Δ	Complexity Δ
...seata/rm/datasource/undo/AbstractUndoExecutor.java	`82.79% <81.66%> (-0.54%)`	`20 <16> (+11)`
...o/seata/rm/datasource/sql/struct/TableRecords.java	`73.68% <0%> (+35.08%)`	`13% <0%> (+4%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 415e5c8...18ea10c. Read the comment docs.

zhangthen

LGTM

zhangthen · 2019-05-17T04:05:38Z

rm-datasource/src/main/java/io/seata/rm/datasource/undo/AbstractUndoExecutor.java

+        // parese pk values
+        Object[] pkValues = parsePkValues(getUndoRows());
+
+        PreparedStatement statement = conn.prepareStatement(checkSQL);


The statement should close in finally .

zhangthen · 2019-05-20T02:32:29Z

rm-datasource/src/main/java/io/seata/rm/datasource/undo/AbstractUndoExecutor.java

     */
-    protected void dataValidation(Connection conn) throws SQLException {
+    protected boolean dataValidation(Connection conn) throws SQLException {
+


It ‘s better to add a switcher to close the dirty data check manually, the switcher is open default.

OK, I will add it in the next standalone PR.

leizhiyuan

I left some comments

leizhiyuan · 2019-05-20T02:37:55Z

rm-datasource/src/test/java/io/seata/rm/datasource/undo/UndoExecutorTest.java

-            executor.executeOn(new MockConnection());
+            // skip data validation
+            Mockito.doReturn(true).when(spy).dataValidation(connection);
+            spy.executeOn(connection);
        } catch (SQLException e) {
            e.printStackTrace();


don't use e.printTrace()

leizhiyuan · 2019-05-20T02:38:01Z

rm-datasource/src/test/java/io/seata/rm/datasource/undo/UndoExecutorTest.java

-            executor.executeOn(new MockConnection());
+            // skip data validation
+            Mockito.doReturn(true).when(spy).dataValidation(connection);
+            spy.executeOn(connection);
        } catch (SQLException e) {
            e.printStackTrace();


don't use e.printTrace()

leizhiyuan · 2019-05-20T03:52:06Z

rm-datasource/src/main/java/io/seata/rm/datasource/undo/AbstractUndoExecutor.java

-        // no need undo if the before data snapshot is equivalent to the after data snapshot.
-        if (DataCompareUtils.isRecordsEquals(sqlUndoLog.getBeforeImage(), sqlUndoLog.getAfterImage())) {
+        // when the data validation is not ok 
+        if (!dataValidation(conn)) {
            return;


should we throw some exception explicitly for the code change later？

Sorry, I don’t get your point.

I add some log and change the method name for more explicitly.

as communicated offline

@leizhiyuan done.

* Do data validation when undo. * Run test case use h2 database. * fix as review. * Add log.

Do data validation when undo.

8ee4266

leizhiyuan and others added 4 commits May 17, 2019 15:28

Merge branch 'develop' into undo_check

d004c1e

Merge remote-tracking branch 'upstream/develop' into undo_check

d2092fd

Run test case use h2 database.

3a8631f

Merge remote-tracking branch 'origin/undo_check' into undo_check

5ff3465

ujjboy changed the title ~~[WIP] Do data validation when undo.~~ Do data validation when undo. May 17, 2019

ujjboy requested a review from zhangthen May 17, 2019 15:42

leizhiyuan requested review from lovepoem and leizhiyuan May 19, 2019 23:26

Merge branch 'develop' into undo_check

128369c

zhangthen approved these changes May 20, 2019

View reviewed changes

leizhiyuan requested changes May 20, 2019

View reviewed changes

ujjboy added 2 commits May 20, 2019 10:46

fix as review.

9d00eb5

Merge branch 'develop' into undo_check

a561bdf

ujjboy requested a review from leizhiyuan May 20, 2019 03:45

leizhiyuan reviewed May 20, 2019

View reviewed changes

ujjboy and others added 3 commits May 20, 2019 13:58

Add log.

9c1d120

Merge branch 'develop' into undo_check

766d3ca

Merge branch 'develop' into undo_check

108afe8

leizhiyuan approved these changes May 21, 2019

View reviewed changes

Merge branch 'develop' into undo_check

18ea10c

leizhiyuan merged commit f6ee500 into apache:develop May 21, 2019

leizhiyuan added this to the 0.6.0 milestone May 21, 2019

ujjboy deleted the undo_check branch May 21, 2019 12:30

fescar-robot mentioned this pull request May 24, 2019

[WeeklyReport] Weekly report for Seata 2019-5-17 to 2019-5-24 #1104

Closed

This was referenced Jun 19, 2019

Rollback Trx No DataValidation #251

Closed

fix #251 add DataValidation #424

Closed

nick-tan pushed a commit to nick-tan/seata that referenced this pull request Jul 12, 2019

Do data validation when undo. (apache#1060)

9677601

* Do data validation when undo. * Run test case use h2 database. * fix as review. * Add log.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do data validation when undo. #1060

Do data validation when undo. #1060

ujjboy commented May 17, 2019

ujjboy commented May 17, 2019

codecov-io commented May 17, 2019 •

edited

zhangthen left a comment

zhangthen May 17, 2019

zhangthen May 20, 2019

ujjboy May 20, 2019

leizhiyuan left a comment

leizhiyuan May 20, 2019

leizhiyuan May 20, 2019

leizhiyuan May 20, 2019

ujjboy May 20, 2019

ujjboy May 20, 2019

leizhiyuan May 20, 2019

ujjboy May 20, 2019

Do data validation when undo. #1060

Do data validation when undo. #1060

Conversation

ujjboy commented May 17, 2019

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

ujjboy commented May 17, 2019

codecov-io commented May 17, 2019 • edited

Codecov Report

zhangthen left a 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

leizhiyuan left a 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

codecov-io commented May 17, 2019 •

edited