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

#751 add event bus mechanism and apply it in tc #768

Merged
merged 10 commits into from
Jun 20, 2019

Conversation

zhengyangyong
Copy link
Contributor

Signed-off-by: zhengyangyong yangyong.zheng@qq.com

#751

@codecov-io
Copy link

codecov-io commented Apr 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@7fefe47). Click here to learn what that means.
The diff coverage is 65.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #768   +/-   ##
==========================================
  Coverage           ?   43.91%           
  Complexity         ?     1447           
==========================================
  Files              ?      253           
  Lines              ?    10262           
  Branches           ?     1332           
==========================================
  Hits               ?     4507           
  Misses             ?     5173           
  Partials           ?      582
Impacted Files Coverage Δ Complexity Δ
...va/io/seata/core/event/GlobalTransactionEvent.java 0% <0%> (ø) 0 <0> (?)
.../java/io/seata/server/coordinator/DefaultCore.java 60.35% <100%> (ø) 24 <9> (?)
...o/seata/server/coordinator/DefaultCoordinator.java 51.54% <100%> (ø) 23 <0> (?)
...c/main/java/io/seata/core/event/GuavaEventBus.java 100% <100%> (ø) 4 <4> (?)
...in/java/io/seata/server/event/EventBusManager.java 66.66% <66.66%> (ø) 1 <1> (?)

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 7fefe47...ec7637b. Read the comment docs.

Copy link
Contributor

@CoffeeLatte007 CoffeeLatte007 left a comment

Choose a reason for hiding this comment

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

Is th pull request for monitoring ?There are two more points to consider:
1.server shutdown or close ,the data will be lost.Whether the data should be persisted and split by day?
2.Whether retrycommit, retryRollback and timeout need to monitored

@zhengyangyong
Copy link
Contributor Author

Is th pull request for monitoring ?There are two more points to consider:
1.server shutdown or close ,the data will be lost.Whether the data should be persisted and split by day?
2.Whether retrycommit, retryRollback and timeout need to monitored

  1. because it is a event bus, I think persistence is not necessary
  2. yes it is a good suggestion I will try add them

@zhengyangyong
Copy link
Contributor Author

  1. Move raise start,commit and rollback event code from DefaultCoordinator to DefaultCore in order to ensure retrycommit and retryRollback monitored
  2. Add timeout event in DefaultCoordinator

@zhengyangyong zhengyangyong force-pushed the tc_event_bus branch 2 times, most recently from 6f8e141 to 83aa6a7 Compare April 18, 2019 07:10
Copy link
Contributor

@CoffeeLatte007 CoffeeLatte007 left a comment

Choose a reason for hiding this comment

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

LGTM

}

@Override
public void register(Object object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe Event should be a custom interface, not object, we can limit the type of event。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, done

@zhengyangyong zhengyangyong force-pushed the tc_event_bus branch 2 times, most recently from 4456284 to 8527afe Compare April 22, 2019 01:49
@leizhiyuan
Copy link
Contributor

LGTM

@xingfudeshi
Copy link
Member

@zhengyangyong there are errors need to be solved.

Signed-off-by: zhengyangyong <yangyong.zheng@qq.com>
…ltCoordinator to DefaultCore in order to ensure retrycommit and retryRollback;add timeout event in DefaultCoordinator

Signed-off-by: zhengyangyong <yangyong.zheng@qq.com>
Signed-off-by: zhengyangyong <yangyong.zheng@qq.com>
… of event

Signed-off-by: zhengyangyong <yangyong.zheng@qq.com>
Signed-off-by: zhengyangyong <yangyong.zheng@qq.com>
import io.seata.core.event.Event;
import io.seata.core.model.GlobalStatus;

public class GlobalTransactionEvent implements Event {
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 move to the core module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes,you are right, Done

@@ -0,0 +1,5 @@
#reduce delay for test
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 necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes ,this file is for test (in test resources folder) in order to reduce asyn-committing-retry-delay (default is 30 second) and timeout-retry-delay (default is 30 second) , then we can fast check event with TimeoutRollbacking status come from core.

@@ -0,0 +1,7 @@
config {
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 necessary?
It seems that it is not consistent with the configuration file of the main project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

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.

For the new file, add the author info.

@zhengyangyong
Copy link
Contributor Author

All new file had add author info

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
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

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

6 participants