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

feature: add prefix counter for NamedThreadFactory #1710

Merged
merged 13 commits into from Oct 21, 2019

Conversation

ggndnn
Copy link
Contributor

@ggndnn ggndnn commented Sep 25, 2019

Ⅰ. Describe what this PR did

Though PR #1576 is one reason of #1702, but it's obviously incomplete. I suspect that there are multiple AsyncCommitting threads executing at the same time causing this test failure.

The reason is that maven-surefire-plugin was used, but parameters reuseForks and forkCount were left to be their default values. According to maven-surefire-plugin doc, only one VM was forked and then reused to execute all tests.

The log below could prove this, notice the UUID used in two test cases:

[INFO] Running io.seata.server.UUIDGeneratorOverflowTest
[UUID 0] is: 4022933692
[UUID 1] is: 4022933693
[UUID 2] is: 4022933694
[UUID 3] is: 4022933695
[UUID 4] is: 4022933696
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.001 s - in io.seata.server.UUIDGeneratorOverflowTest
[INFO] Running io.seata.server.event.DefaultCoreForEventBusTest
2019-09-23 10:28:11.754 INFO [main]io.seata.server.coordinator.DefaultCore.begin:142 -Successfully begin global transaction xid = 172.17.0.1:0:4022933697

Because several test case in seata didn't release resources in time, for example, in test case DefaultCoordinatorMetricsTest, DefaultCoordinator#init was called, but never destroyed, that when DefaultCoreForEventBusTest started, there may be more than one asyncCommitting threads running.

It's not so easy to reproduce this test failure, so this PR is opened to verify this suspicion, just waiting for the next failure. I'm not sure whether this suspicion is correct, if it proves right, all test cases should be checked and try to release resources created correctly.

This suspicion proves correct, if we can get log looks like below, please notice the thread name:

2019-09-23 10:28:11.850 INFO [AsyncCommitting_1_1]io.seata.server.coordinator.DefaultCore.doGlobalCommit:245 -Global[172.17.0.1:0:4022933697] committing is successfully done.
2019-09-23 10:28:11.851 INFO [AsyncCommitting_2_1]io.seata.server.coordinator.DefaultCore.doGlobalCommit:245 -Global[172.17.0.1:0:4022933697] committing is successfully done.
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.042 s <<< FAILURE! - in io.seata.server.event.DefaultCoreForEventBusTest
[ERROR] test  Time elapsed: 1.041 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
	at io.seata.server.event.DefaultCoreForEventBusTest.test(DefaultCoreForEventBusTest.java:82)
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   DefaultCoreForEventBusTest.test:82 expected: <1> but was: <2>
[INFO] 
[ERROR] Tests run: 90, Failures: 1, Errors: 0, Skipped: 1

Update(2019/10/17):

As the issue #1702 has been solved, change to a feature PR, it's helpful to investigate threads related issues.

Ⅱ. 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 Sep 25, 2019

Codecov Report

Merging #1710 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1710      +/-   ##
=============================================
- Coverage      50.73%   50.71%   -0.02%     
- Complexity      2162     2163       +1     
=============================================
  Files            414      414              
  Lines          14153    14156       +3     
  Branches        1700     1700              
=============================================
- Hits            7180     7179       -1     
- Misses          6284     6287       +3     
- Partials         689      690       +1
Impacted Files Coverage Δ Complexity Δ
...ava/io/seata/common/thread/NamedThreadFactory.java 90.47% <100%> (+1.58%) 6 <1> (+1) ⬆️
...server/store/file/FileTransactionStoreManager.java 59.21% <0%> (-0.99%) 29% <0%> (ø)
...very/registry/zk/ZookeeperRegisterServiceImpl.java 63.77% <0%> (-0.79%) 25% <0%> (ø)

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 b78f425...0609821. Read the comment docs.

Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

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

LGTM.

@ggndnn ggndnn changed the title test: verify that the analysis of the cause of issue 1702 is correct feature: add prefix counter for NamedThreadFactory Oct 16, 2019
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

@xingfudeshi xingfudeshi merged commit fcf6065 into apache:develop Oct 21, 2019
dangquocbang pushed a commit to dangquocbang/seata that referenced this pull request Oct 22, 2019
* 'develop' of https://github.com/seata/seata:
  feature:support gRpc (apache#1492)
  bugfix:fix auto proxy for multiple-datasource does not work (apache#1793)
  optimize: increase rm code coverage by db mock (apache#1674)
  bugfix: mysql can not get primary key value (apache#1788)
  bugfix: jdk 11 remoteAddress is null (apache#1764)
  test: verify that the analysis of the cause of issue 1702 is correct (apache#1710)
  optimize: seata server register eureka instance id (apache#1790)
  optimize: put message to logQueue (apache#1760)
  optimize:optimize rpc remoting log(apache#1787)
  optimize: simplify code (apache#1786)
  version: change version to 1.0.0-SNAPSHOT (apache#1785)
  bugfix: clean up resources in time to avoid mutual influence between unit tests (apache#1778)
  optimize:remove unused method (apache#1766)
  bugfix: DeleteExecutor. buildBeforeImageSQL keyword checker by db type (apache#1777)
  optimize: string splice and release lock (apache#1770)
  release 0.9.0 (apache#1771)
  bugfix: oracle rollback failed when the table has null Blob Clob value (apache#1761)
  bugfix: service method not support interface type parameter (apache#1759)
  optimize: abstract table meta cache (apache#1750)
  optimize: improve the efficiency of the batch log (apache#1743)
@ggndnn ggndnn mentioned this pull request May 28, 2020
1 task
@ggndnn ggndnn deleted the test_issue_1702 branch May 30, 2020 05:41
@slievrly slievrly added this to the 1.3.0 milestone Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants