Feature/sentinel pigeon adapter #781

wchswchs · 2019-05-21T11:50:19Z

Describe what this PR does / why we need it

This PR provides RPC framework Pigeon adapter for Sentinel.

Does this pull request fix one issue?

Features #780

Describe how you did it

Add default fallback and provider & invoker interceptor

Describe how to verify it

Run sentinel-demo-pigeon

Special notes for reviews

CLAassistant · 2019-05-21T11:50:29Z

All committers have signed the CLA.

codecov-io · 2019-05-21T12:01:41Z

Codecov Report

Merging #781 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #781      +/-   ##
============================================
+ Coverage     41.72%   41.73%   +0.01%     
  Complexity     1377     1377              
============================================
  Files           304      304              
  Lines          8764     8764              
  Branches       1182     1182              
============================================
+ Hits           3657     3658       +1     
  Misses         4660     4660              
+ Partials        447      446       -1

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/alibaba/csp/sentinel/node/ClusterNode.java	`100% <0%> (+4.76%)`	`8% <0%> (+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 281d342...67bdf22. Read the comment docs.

add pigeon adapter

jasonjoo2010

Using only english and describing clearly in commits' history maybe better.

jasonjoo2010

And maybe we should add tests to save the poor coverage. Please reference to sentinel-dubbo-adapter

jasonjoo2010 · 2019-05-21T16:39:04Z

sentinel-adapter/sentinel-pigeon-adapter/pom.xml

+
+    <properties>
+        <java.source.version>1.8</java.source.version>
+        <java.target.version>1.8</java.target.version>


Why set the target to java8? Is there a library requiring this?

Target version is set to 1.7 in parent.

Added test & remove jdk version 1.8 properties

wchswchs · 2019-05-22T02:13:12Z

Using only english and describing clearly in commits' history maybe better.

ok，next commit begin to use en.

…wchswchs/Sentinel into feature/sentinel-pigeon-adapter

sczyh30 · 2019-05-23T16:47:48Z

.../src/main/java/com/alibaba/csp/sentinel/adapter/pigeon/SentinelPigeonInvokerInterceptor.java

+    }
+
+    @Override
+    public void afterThrowing(InvokerContext invokerContext, Throwable throwable) {


I'm not familiar with Pigeon, but I guess the afterThrowing will be triggered when exceptions are thrown. We need to trace the exception here using Tracer.traceEntry(ex, entry). And if the postInvoke is not invoked later, we need to exit the entry here.

I reviewed pigeon code, you are right, afterThrowing will be invoded when method invoked throwing exceptions, so trace the exception needed to be placed into afterThrowing. And postInvoke will be invoked after method invoked, so exit entry needed to be placed into postInvoke. I will resolve these problem at once.

sczyh30 · 2019-05-23T16:51:32Z

.../src/main/java/com/alibaba/csp/sentinel/adapter/pigeon/SentinelPigeonInvokerInterceptor.java

+
+    @Override
+    public void preInvoke(InvokerContext invokerContext) {
+        Entry methodEntry = null;


In my understanding, the response time should be calculated between preInvoke and postInvoke hook, so here if we exit the entry in preInvoke, the RT will be wrong. The context, as well as the invocation chain, would also not be kept. You'll need to call entry.exit() in postInvoke hook. You may need to carry the Entry via InvokerContext.

I have fixed these 2 problems.

sczyh30 · 2019-05-26T02:50:55Z

.../src/main/java/com/alibaba/csp/sentinel/adapter/pigeon/SentinelPigeonInvokerInterceptor.java

+
+    @Override
+    public void postInvoke(InvokerContext invokerContext) {
+        Entry methodEntry = ContextUtil.getContext().getCurEntry();


Can we guarantee the preInvoke and postInvoke of one invocation are invoked in the same thread? If they were called in different threads, the context would be wrong.

Maybe we can carry the Entry in InvokerContext?

I'm sure preInvoke and postInvoke of one invocation are invoked in the same thread, because of the attribute of InvokeContext in Pigeon must be Serializable, but Entry is not Serializable now and I think Entry don't need to Serializable, so i use ContextUtil to get current Entry.

sczyh30 · 2019-05-31T04:52:34Z

sentinel-demo/sentinel-demo-pigeon/pom.xml

+    <artifactId>sentinel-demo-pigeon</artifactId>
+
+    <repositories>
+        <repository>


Maybe it's not necessary to add the snapshot repository here?

I have removed snapshot.

Why is github-dianping-maven-repo is needed here? I've tried the URL and it's 400 bad request.

Because pigeon dependency can be fetched only from github-dianping-maven-repo.

Why is github-dianping-maven-repo is needed here? I've tried the URL and it's 400 bad request.

You can compiled the root of sentinel, it's successful.

sczyh30 · 2019-05-31T04:53:35Z

sentinel-demo/sentinel-demo-pigeon/pom.xml

+        <!-- Dependency for facebook -->
+        <dependency>
+            <groupId>com.facebook.swift</groupId>
+            <artifactId>swift-annotations</artifactId>


Do the dependencies about com.facebook.swift make sense?

Pigeon provider initialization need to swift dependency, so I add facebook dependencies related to pom.

sczyh30 · 2019-06-04T11:49:37Z

I've noticed that the com.dianping:pigeon module is absent in Maven Central and can only be retrieved from a GitHub repo, which is not a standard case. Maybe it's more appropriate to be in an individual repo (e.g. create a new sentinel-pigeon-adapter repo under sentinel-group). You can submit a PR to add the repo link in awesone-sentinel.md.

wchswchs · 2019-06-05T04:53:57Z

I've noticed that the com.dianping:pigeon module is absent in Maven Central and can only be retrieved from a GitHub repo, which is not a standard case. Maybe it's more appropriate to be in an individual repo (e.g. create a new sentinel-pigeon-adapter repo under sentinel-group). You can submit a PR to add the repo link in awesone-sentinel.md.

ok, I have added sentinel-pigeon-adapter into awesome.

sczyh30 · 2019-06-05T22:05:38Z

It might be better if you could create a new repo sentinel-pigeon-adapter and put your module into it (without other parts of Sentinel), then link in awesone-sentinel.md, which can be more clear :)

…pigeon-adapter

wchswchs · 2019-06-07T12:28:12Z

I have removed sentinel-pigeon-adapter from sentinel-adapter and linked sentinel-pigeon-adapter to my repo.

sczyh30 · 2019-06-10T02:05:18Z

But I think pigeon integration is equally important to dubbo, why couldn't be placed into sentinel-adapter?

Yes it's indeed equally important to Dubbo, but it's not deployed to Maven Central and its way to manage libs is confusing (and not standard). We might only accept the libraries that has been deployed to Maven Central or else valid Maven repositories (e.g. Spring repo).

sczyh30 · 2019-06-11T08:10:29Z

Now that the adapter code has been migrated to sentinel-pigeon-adapter repo, please also remove the relevant code in this PR and only keep the change for awesome-sentinel.md.

sczyh30 · 2019-06-11T08:12:15Z

By the way, would you like to donate sentinel-pigeon-adapter to sentinel-group org? You could join the org and manage your repo here.

wchswchs · 2019-06-11T08:16:25Z

Now that the adapter code has been migrated to sentinel-pigeon-adapter repo, please also remove the relevant code in this PR and only keep the change for awesome-sentinel.md.

I have removed the relevant code with pigeon adapter.

wchswchs · 2019-06-11T08:24:03Z

By the way, would you like to donate sentinel-pigeon-adapter to sentinel-group org? You could join the org and manage your repo here.

How should I do?

sczyh30 · 2019-06-11T08:52:33Z

By the way, would you like to donate sentinel-pigeon-adapter to sentinel-group org? You could join the org and manage your repo here.

How should I do?

You could refer to https://help.github.com/en/articles/transferring-a-repository. The target org is sentinel-group.

wchswchs · 2019-06-11T08:54:34Z

By the way, would you like to donate sentinel-pigeon-adapter to sentinel-group org? You could join the org and manage your repo here.

How should I do?

You could refer to https://help.github.com/en/articles/transferring-a-repository. The target org is sentinel-group.

ok

sczyh30

LGTM

sczyh30 · 2019-06-11T09:03:10Z

Thanks for contributing. Once the transfer is finished, I'll update the awesome-sentinel.md :)

wchswchs · 2019-06-11T09:06:38Z

Thanks for contributing. Once the transfer is finished, I'll update the awesome-sentinel.md :)

When I transfer the repo to sentinel-group, it tells me you don’t have the permission to create repositories on sentinel-group.

sczyh30 · 2019-06-11T09:10:26Z

Thanks for contributing. Once the transfer is finished, I'll update the awesome-sentinel.md :)

When I transfer the repo to sentinel-group, it tells me you don’t have the permission to create repositories on sentinel-group.

I've invited you to join https://github.com/sentinel-group. Once you've joined you could transfer the repo.

wchswchs · 2019-06-11T09:14:07Z

Thanks for contributing. Once the transfer is finished, I'll update the awesome-sentinel.md :)

When I transfer the repo to sentinel-group, it tells me you don’t have the permission to create repositories on sentinel-group.

I've invited you to join https://github.com/sentinel-group. Once you've joined you could transfer the repo.

I have transfered sentinel-pigeon to sentinel-group. :)

sczyh30 · 2019-06-11T09:15:07Z

Nice, great work! 👍

[RIP-9] Added sample English documents of English transaction messages

jasonjoo2010 force-pushed the feature/sentinel-pigeon-adapter branch from 7fe6827 to 834caec Compare May 21, 2019 13:09

add pigeon adapter

b838dde

jasonjoo2010 force-pushed the feature/sentinel-pigeon-adapter branch from 834caec to b838dde Compare May 21, 2019 13:16

wchswchs added 2 commits May 21, 2019 22:18

合并commit

01f4075

add pigeon adapter

add license

1b34855

wchswchs force-pushed the feature/sentinel-pigeon-adapter branch from b838dde to 1b34855 Compare May 21, 2019 14:22

sczyh30 added area/integrations Issues or PRs related to integrations with open-source components to-review To review labels May 21, 2019

jasonjoo2010 reviewed May 21, 2019

View reviewed changes

wchswchs added 6 commits May 22, 2019 14:20

add test & remove jdk version 1.8

d32ea0c

add pigeon adapter

45279cf

fix ci error

df6b88e

add pigeon adapter

30b2b50

Merge branch 'feature/sentinel-pigeon-adapter' of https://github.com/…

f99aeb4

…wchswchs/Sentinel into feature/sentinel-pigeon-adapter

add license

8bc250c

sczyh30 requested changes May 23, 2019

View reviewed changes

throw exception trace & calculate rt problem fix

df70e02

sczyh30 reviewed May 26, 2019

View reviewed changes

sczyh30 reviewed May 31, 2019

View reviewed changes

remove snapshot

dbaa78f

wchswchs closed this Jun 5, 2019

wchswchs reopened this Jun 5, 2019

add pigeon adapter introduction to awesome

c769933

upgrade to 1.7.0

1aa86a8

wchswchs added 3 commits June 7, 2019 20:24

Merge remote-tracking branch 'upstream/master' into feature/sentinel-…

05fc689

…pigeon-adapter

remove pigeon adapter

f3fe269

update pigeon adapter doc

82afb33

wchswchs closed this Jun 7, 2019

wchswchs reopened this Jun 7, 2019

wchswchs added 2 commits June 11, 2019 16:05

resolve doc/awesome-sentinel.md conflict && merge

0363092

remove duplicated code

04cf686

remove pigeon adapter relevant code

67bdf22

sczyh30 approved these changes Jun 11, 2019

View reviewed changes

sczyh30 merged commit e3b9f99 into alibaba:master Jun 11, 2019

sczyh30 removed the to-review To review label Jun 11, 2019

sczyh30 mentioned this pull request Jun 11, 2019

Sentinel Pigeon Adapter #780

Closed

sentinel-bot mentioned this pull request Jun 14, 2019

[WeeklyReport] Weekly report for Sentinel 2019-6-7 to 2019-6-14 #834

Closed

pouchrobot mentioned this pull request Jun 14, 2019

WeeklyReport of Sentinel from 2019-06-07 to 2019-06-14 #835

Closed

CST11021 pushed a commit to CST11021/Sentinel that referenced this pull request Nov 3, 2021

Merge pull request alibaba#781 from chenhaoxiang/develop

e01b5ae

[RIP-9] Added sample English documents of English transaction messages

Feature/sentinel pigeon adapter #781

Feature/sentinel pigeon adapter #781

Conversation

wchswchs commented May 21, 2019 • edited Loading

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

CLAassistant commented May 21, 2019 • edited Loading

codecov-io commented May 21, 2019 • edited Loading

Codecov Report

jasonjoo2010 left a comment

Choose a reason for hiding this comment

jasonjoo2010 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchswchs commented May 22, 2019

Choose a reason for hiding this comment

wchswchs May 24, 2019 • edited Loading

Choose a reason for hiding this comment

sczyh30 May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchswchs May 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sczyh30 May 31, 2019 • edited Loading

Choose a reason for hiding this comment

wchswchs Jun 1, 2019 • edited Loading

Choose a reason for hiding this comment

sczyh30 commented Jun 4, 2019 • edited Loading

wchswchs commented Jun 5, 2019 • edited Loading

sczyh30 commented Jun 5, 2019

wchswchs commented Jun 7, 2019 • edited Loading

sczyh30 commented Jun 10, 2019

sczyh30 commented Jun 11, 2019

sczyh30 commented Jun 11, 2019

wchswchs commented Jun 11, 2019

wchswchs commented Jun 11, 2019

sczyh30 commented Jun 11, 2019

wchswchs commented Jun 11, 2019 • edited Loading

sczyh30 left a comment

Choose a reason for hiding this comment

sczyh30 commented Jun 11, 2019

wchswchs commented Jun 11, 2019

sczyh30 commented Jun 11, 2019

wchswchs commented Jun 11, 2019

sczyh30 commented Jun 11, 2019

wchswchs commented May 21, 2019 •

edited

Loading

CLAassistant commented May 21, 2019 •

edited

Loading

codecov-io commented May 21, 2019 •

edited

Loading

wchswchs May 24, 2019 •

edited

Loading

sczyh30 May 23, 2019 •

edited

Loading

wchswchs May 27, 2019 •

edited

Loading

sczyh30 May 31, 2019 •

edited

Loading

wchswchs Jun 1, 2019 •

edited

Loading

sczyh30 commented Jun 4, 2019 •

edited

Loading

wchswchs commented Jun 5, 2019 •

edited

Loading

wchswchs commented Jun 7, 2019 •

edited

Loading

wchswchs commented Jun 11, 2019 •

edited

Loading