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: fix the NPE and reduce the request when lockkey is null #2495

Merged
merged 8 commits into from
Apr 8, 2020
Merged

bugfix: fix the NPE and reduce the request when lockkey is null #2495

merged 8 commits into from
Apr 8, 2020

Conversation

lightClouds917
Copy link
Contributor

@lightClouds917 lightClouds917 commented Apr 1, 2020

Ⅰ. Describe what this PR did

1.fix: Fix the NPE when lockkey is null;
2.optimize: Reduce once db request when lockkey is null;

Ⅱ. Does this pull request fix one issue?

yes.
issue

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@zjinlei zjinlei added the bug label Apr 1, 2020
@zjinlei zjinlei added this to the 1.2.0 milestone Apr 1, 2020

checkLock(context.buildLockKeys());
Set<String> lockKeys = context.getLockKeysBuffer();
if (CollectionUtils.isNotEmpty(lockKeys)) {
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 more appropriate to put logic with lockkey as empty in the checkLock method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,i will change

Choose a reason for hiding this comment

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

image
the master code

@codecov-io
Copy link

codecov-io commented Apr 3, 2020

Codecov Report

Merging #2495 into develop will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2495      +/-   ##
=============================================
- Coverage      51.45%   51.42%   -0.03%     
+ Complexity      2666     2664       -2     
=============================================
  Files            529      529              
  Lines          16956    16964       +8     
  Branches        2051     2052       +1     
=============================================
  Hits            8724     8724              
- Misses          7408     7412       +4     
- Partials         824      828       +4     
Impacted Files Coverage Δ Complexity Δ
...n/java/io/seata/rm/datasource/ConnectionProxy.java 25.61% <0.00%> (-0.44%) 12.00 <0.00> (ø)
...java/io/seata/server/lock/AbstractLockManager.java 60.31% <0.00%> (-4.09%) 13.00 <1.00> (ø)
...o/seata/server/storage/db/lock/DataBaseLocker.java 15.55% <0.00%> (-0.73%) 5.00 <1.00> (ø)
...o/seata/server/coordinator/DefaultCoordinator.java 54.08% <0.00%> (-0.52%) 29.00% <0.00%> (-2.00%)
...in/java/io/seata/server/session/GlobalSession.java 85.32% <0.00%> (-0.46%) 71.00% <0.00%> (-1.00%)
...torage/file/store/FileTransactionStoreManager.java 57.14% <0.00%> (+0.63%) 30.00% <0.00%> (+1.00%)

@slievrly slievrly changed the title fix:fix the NPE,and reduce once db request when lockkey is null bugfix: fix the NPE and reduce once db request when lockkey is null Apr 3, 2020
@slievrly slievrly changed the title bugfix: fix the NPE and reduce once db request when lockkey is null bugfix: fix the NPE and reduce the request when lockkey is null Apr 3, 2020
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

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

@@ -153,6 +153,9 @@ protected Locker getLocker() {
Long branchID) {
List<RowLock> locks = new ArrayList<RowLock>();

if (StringUtils.isEmpty(lockKey)) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect

Copy link
Member

Choose a reason for hiding this comment

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

check isLockable method.

@@ -153,6 +153,9 @@ protected Locker getLocker() {
Long branchID) {
List<RowLock> locks = new ArrayList<RowLock>();

if (StringUtils.isEmpty(lockKey)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That s ok. And I think can remove the code

if (StringUtils.isNullOrEmpty(lockKey)) {
            // no lock
            return true;
}

at

acquireLock(BranchSession branchSession)

Copy link
Contributor

@l81893521 l81893521 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 merged commit 2e37c6b into apache:develop Apr 8, 2020
@wangliang181230 wangliang181230 added type: bug Category issues or prs related to bug. and removed bug labels Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Category issues or prs related to bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants