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

optimize: distinguish database behavior according to the branch type #2415

Merged
merged 39 commits into from Apr 27, 2020

Conversation

booogu
Copy link
Contributor

@booogu booogu commented Mar 17, 2020

Ⅰ. Describe what this PR did

1.Change the interceptorType to branchType,optimize the logic in TccActionInterceptor
2.Optimize the judge condition of use UndoFunction in AT's ExecuteTemplate

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Mar 17, 2020

Codecov Report

Merging #2415 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #2415   +/-   ##
==========================================
  Coverage      51.20%   51.20%           
  Complexity      2811     2811           
==========================================
  Files            554      554           
  Lines          17774    17774           
  Branches        2100     2100           
==========================================
  Hits            9102     9102           
  Misses          7812     7812           
  Partials         860      860           

@zjinlei zjinlei added this to the 1.2.0 milestone Mar 18, 2020
@booogu
Copy link
Contributor Author

booogu commented Mar 19, 2020

PTAL @zjinlei @slievrly

@zjinlei zjinlei removed this from the 1.2.0 milestone Apr 10, 2020
@@ -66,7 +68,7 @@
StatementCallback<T, S> statementCallback,
Object... args) throws SQLException {

if (!RootContext.inGlobalTransaction() && !RootContext.requireGlobalLock()) {
if (!requiresUndoFunction()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@GlobalLock does not have undo, this function name should be optimized

} else {
if (rpcXid != null) {
RootContext.bind(rpcXid);
RootContext.bindInterceptorType(rpcXidInterceptorType);
if (StringUtils.equals(BranchType.TCC.name(), rpcBranchType)) {
RootContext.bindBranchType(BranchType.TCC);
Copy link
Contributor

Choose a reason for hiding this comment

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

Saga branch never set ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have discussed with the author of saga. This pr is only responsible for the branchType judgment logic in ExecuteTemplate and the TCC branchType passing in Filter. The specific Saga branchtype assignment and other logic will be provided by the feature saga related pr

Copy link
Contributor

Choose a reason for hiding this comment

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

this #2551, autor is wangliang1986

Copy link
Contributor

@long187 long187 left a comment

Choose a reason for hiding this comment

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

LGTM

@booogu booogu changed the title optimize: optimize interceptorType to branchType optimize: distinguish database behavior according to the branch type Apr 24, 2020
Copy link
Contributor

@wangliang181230 wangliang181230 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zjinlei zjinlei left a comment

Choose a reason for hiding this comment

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

LGTM

@zjinlei zjinlei added the Do Not Merge Do not merge into develop label Apr 25, 2020
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("xid in RootContext[{}] xid in RpcContext[{}]", xid, rpcXid);
}
boolean bind = false;
if (xid != null) {
RpcContext.getContext().setAttachment(RootContext.KEY_XID, xid);
RpcContext.getContext().setAttachment(RootContext.KEY_XID_INTERCEPTOR_TYPE, xidInterceptorType);
RpcContext.getContext().setAttachment(RootContext.KEY_BRANCH_TYPE, branchType);
Copy link
Member

Choose a reason for hiding this comment

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

Is it better to separate and judge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not be processed this time, and transaction context object encapsulation will be provided later to handle log output uniformly.

if (LOGGER.isDebugEnabled()) {
LOGGER.debug("unbind[{}] interceptorType[{}] from RootContext", unbindXid, unbindInterceptorType);
LOGGER.debug("unbind xid [{}] branchType [{}] from RootContext", unbindXid, previousBranchType);
Copy link
Member

Choose a reason for hiding this comment

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

BranchType print null in AT mode is not very elegant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about judge in the log output temporarily,and print "AT" if it is null

@@ -115,4 +117,17 @@
}
return rs;
}

private static boolean requiresLockOrUndoFunction() {
Copy link
Member

Choose a reason for hiding this comment

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

Other more meaningful method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change it to "shouldExecuteInATMode"

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

@zjinlei zjinlei merged commit e5c68a7 into apache:develop Apr 27, 2020
@wangliang181230
Copy link
Contributor

此PR合并后,出现的BUG: 在TCC或SAGA事务内,还执行了SQL识别功能,导致SAGA事务执行失败。
代码行:AbstractConnectionProxy类的PreparedStatement prepareStatement(String sql)方法,第112行。
@Override public PreparedStatement prepareStatement(String sql) throws SQLException { String dbType = getDbType(); // support oracle 10.2+ PreparedStatement targetPreparedStatement = null; if (RootContext.inGlobalTransaction()) { List<SQLRecognizer> sqlRecognizers = SQLVisitorFactory.get(sql, dbType); if (sqlRecognizers != null && sqlRecognizers.size() == 1) { SQLRecognizer sqlRecognizer = sqlRecognizers.get(0); if (sqlRecognizer != null && sqlRecognizer.getSQLType() == SQLType.INSERT) { String tableName = ColumnUtils.delEscape(sqlRecognizer.getTableName(), dbType); TableMeta tableMeta = TableMetaCacheFactory.getTableMetaCache(dbType).getTableMeta(getTargetConnection(), tableName, getDataSourceProxy().getResourceId()); targetPreparedStatement = getTargetConnection().prepareStatement(sql, new String[]{tableMeta.getPkName()}); } } } if (targetPreparedStatement == null) { targetPreparedStatement = getTargetConnection().prepareStatement(sql); } return new PreparedStatementProxy(this, targetPreparedStatement, sql); }

中的RootContext.inGlobalTransaction()是否应该改为ExecuteTemplate.shouldExecuteInATMode()。

@booogu
Copy link
Contributor Author

booogu commented Apr 29, 2020

此PR合并后,出现的BUG: 在TCC或SAGA事务内,还执行了SQL识别功能,导致SAGA事务执行失败。
代码行:AbstractConnectionProxy类的PreparedStatement prepareStatement(String sql)方法,第112行。
@Override public PreparedStatement prepareStatement(String sql) throws SQLException { String dbType = getDbType(); // support oracle 10.2+ PreparedStatement targetPreparedStatement = null; if (RootContext.inGlobalTransaction()) { List<SQLRecognizer> sqlRecognizers = SQLVisitorFactory.get(sql, dbType); if (sqlRecognizers != null && sqlRecognizers.size() == 1) { SQLRecognizer sqlRecognizer = sqlRecognizers.get(0); if (sqlRecognizer != null && sqlRecognizer.getSQLType() == SQLType.INSERT) { String tableName = ColumnUtils.delEscape(sqlRecognizer.getTableName(), dbType); TableMeta tableMeta = TableMetaCacheFactory.getTableMetaCache(dbType).getTableMeta(getTargetConnection(), tableName, getDataSourceProxy().getResourceId()); targetPreparedStatement = getTargetConnection().prepareStatement(sql, new String[]{tableMeta.getPkName()}); } } } if (targetPreparedStatement == null) { targetPreparedStatement = getTargetConnection().prepareStatement(sql); } return new PreparedStatementProxy(this, targetPreparedStatement, sql); }

中的RootContext.inGlobalTransaction()是否应该改为ExecuteTemplate.shouldExecuteInATMode()。

抱歉给您带来了问题,是的,确实有这个问题,虽然执行的时候不进行sql解析构建undoLog了,但是这里还是会执行sql识别,这是我的疏忽,我马上提交一个修复pr

@wangliang181230
Copy link
Contributor

OK

@booogu
Copy link
Contributor Author

booogu commented May 2, 2020

OK
I fix it in this pr,please take a look.
#2650

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge Do not merge into develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants