Skip to content
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

bugfix: when use Statement.executeBatch() api can not generate after … #4838

Merged
merged 7 commits into from
Aug 6, 2022

Conversation

a1104321118
Copy link
Contributor

@a1104321118 a1104321118 commented Aug 3, 2022

…image and undo log

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

fix when use Statement.executeBatch() api can not generate undo log

Ⅱ. Does this pull request fix one issue?

fixes #4833

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@funky-eyes funky-eyes added type: bug Category issues or prs related to bug. first-time contributor first-time contributor module/rm-datasource rm-datasource module labels Aug 3, 2022
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 请将作者与pr信息登记至changes文件夹中的中英两版develop.md中

@funky-eyes funky-eyes added this to the 1.6.0 milestone Aug 3, 2022
TableRecords afterImage = afterImage(beforeImage);
prepareUndoLog(beforeImage, afterImage);
}
return result;
}

protected boolean needUndoLog() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQLexception

@funky-eyes
Copy link
Contributor

我建议直接删了这个判断影响行数先吧,或者你可以进行标记batch的动作再来决定是否进行相关判断

@@ -126,6 +131,11 @@ public void addBatch(String sql) throws SQLException {

@Override
public int[] executeBatch() throws SQLException {
this.executeBatchApiUsed = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statement can be reuse,the variable can be reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statement can be reuse,the variable can be reset?

emmm, 这个变量是用于判断是否曾经执行过 executeBatch 方法的,只要执行过就标记为 true;所以没有考虑是否需要恢复的问题; 当然,这里主要是为了解决 issue 对应的问题,或者有什么更好的方案吗? 因为我不确定把它 reset 了之后,未来会不会对后续的其它操作产生影响

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不重置,后续的statement执行时update(影响行数为0)时,也会生成后置镜像

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议直接删除影响行数判断

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议直接删除影响行数判断

好的,那这个 pr 我先删掉吧,等我研究出更好的方案再提个优化的 pr 好了

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议直接删除影响行数判断

好的,那这个 pr 我先删掉吧,等我研究出更好的方案再提个优化的 pr 好了

可以的

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

Merging #4838 (57d960d) into develop (350ce31) will increase coverage by 0.20%.
The diff coverage is 28.57%.

❗ Current head 57d960d differs from pull request most recent head 99375e2. Consider uploading reports for the commit 99375e2 to get more accurate results

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #4838      +/-   ##
=============================================
+ Coverage      49.20%   49.40%   +0.20%     
- Complexity      4090     4116      +26     
=============================================
  Files            736      736              
  Lines          25723    25728       +5     
  Branches        3177     3178       +1     
=============================================
+ Hits           12658    12712      +54     
+ Misses         11716    11667      -49     
  Partials        1349     1349              
Impacted Files Coverage Δ
...ta/rm/datasource/exec/AbstractDMLBaseExecutor.java 51.78% <20.00%> (-2.94%) ⬇️
...in/java/io/seata/rm/datasource/StatementProxy.java 96.77% <50.00%> (-3.23%) ⬇️
...erver/storage/file/session/FileSessionManager.java 47.77% <0.00%> (-0.64%) ⬇️
...torage/file/store/FileTransactionStoreManager.java 56.91% <0.00%> (+0.64%) ⬆️
...rage/redis/store/RedisTransactionStoreManager.java 67.70% <0.00%> (+4.53%) ⬆️
...n/src/main/java/io/seata/common/util/IdWorker.java 83.33% <0.00%> (+6.25%) ⬆️
...java/io/seata/server/storage/SessionConverter.java 89.09% <0.00%> (+9.09%) ⬆️
...va/io/seata/server/console/vo/GlobalSessionVO.java 55.88% <0.00%> (+33.82%) ⬆️

Copy link
Member

@jsbxyyx jsbxyyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jsbxyyx jsbxyyx merged commit 5b15922 into apache:develop Aug 6, 2022
@a1104321118 a1104321118 deleted the bugfix-need-undo-log-judgement branch August 8, 2022 01:53
@a1104321118 a1104321118 restored the bugfix-need-undo-log-judgement branch August 8, 2022 01:53
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor module/rm-datasource rm-datasource module multilingual type: bug Category issues or prs related to bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Statement.executeBatch() Api To InsertBatch Cannot generate UndoLog
6 participants