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

Supporting RequestRateLimiterGatewayFilterFactory #3538

Merged
merged 28 commits into from
Oct 22, 2019
Merged

Supporting RequestRateLimiterGatewayFilterFactory #3538

merged 28 commits into from
Oct 22, 2019

Conversation

xiaoy00
Copy link
Contributor

@xiaoy00 xiaoy00 commented Sep 27, 2019

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • [1] Bug fix

  • New feature provided

  • Improve performance

  • Related issues
    no


Bug fix

  • Bug description.
    when i use the RequestRateLimiterGatewayFilterFactory for limit.
    then ContextManager.capture() can not work in NettyRoutingFilterInterceptor.class
    because Thread is changed from 'reactor-http-nio' to 'lettuce-nioEventLoop'

  • How to fix?
    get ContextSnapshot on FilteringWebHandlerInterceptor and set it into SWTransmitter.
    then NettyRoutingFilterInterceptor can use ContextSnapshot to ContextManager.continued(swTransmitter.getSnapshot());
    and i have to create a new LocalSpan for bring ContextManager.continued() worked


New feature or improvement

  • Describe the details and related test reports.

@wu-sheng wu-sheng added agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Sep 27, 2019
@wu-sheng wu-sheng added this to the 6.5.0 milestone Sep 27, 2019
@wu-sheng
Copy link
Member

@xiaoy00 Could you provide demo codes about this?

@zhaoyuguang
Copy link
Member

The current gateway test demo, it would be better to enrich this case here.

@xiaoy00
Copy link
Contributor Author

xiaoy00 commented Sep 29, 2019

The current gateway test demo, it would be better to enrich this case here.

I have already submitted the case on gateway test demo, please have a look

@zhaoyuguang
Copy link
Member

zhaoyuguang commented Sep 30, 2019

Hi @xiaoy00 , Please modify the code according to code style and modify testcase so that test cases can work.

@xiaoy00
Copy link
Contributor Author

xiaoy00 commented Sep 30, 2019

Hi @xiaoy00 , Please modify the code according to code style and modify testcase so that test cases can work.
you talk about gateway-2.1.x-plugin or gateway test demo ?

@zhaoyuguang
Copy link
Member

Fix gateway-2.1.x-plugin according to code style.
And make gateway test demo can work.

@xiaoy00
Copy link
Contributor Author

xiaoy00 commented Sep 30, 2019

Fix gateway-2.1.x-plugin according to code style.
And make gateway test demo can work.
check style is ok on my idea.
demo can work too, by the way , its need redis to work.
i dont know what's the error . and i showed you on QQ.
u can give me some error infos.

@wu-sheng
Copy link
Member

You should verify style by maven command. It is what we used on CI. Also, click details link, we have the logs.

@wu-sheng
Copy link
Member

The referred demo is integratuon test, you need to read the plugin tests document and change the codes, then make tests passed.

In open source, we need to make sure cases are checked in any further changes. But not just your local tests.
Please understand, we need to make sure the whole community good, and make sure all upstream changes are safe and could be reverified always.
You need to spend mire time on tests than code changes.

@wu-sheng wu-sheng removed this from the 6.5.0 milestone Sep 30, 2019
@apache apache deleted a comment from asf-ci Oct 3, 2019
@wu-sheng
Copy link
Member

#3603 merged. Please continue.

@xiaoy00
Copy link
Contributor Author

xiaoy00 commented Oct 15, 2019

#3603 merged. Please continue.
roger!

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

There are many filed not under Apache 2.0 license. So CI fail with rat check unpassed. Please fix.

@zhaoyuguang Please review the code changes.

@wu-sheng
Copy link
Member

Resolve the conflicts, please.

@zhaoyuguang
Copy link
Member

/run agent-plugin-test-3

@xiaoy00
Copy link
Contributor Author

xiaoy00 commented Oct 21, 2019

/run agent-plugin-test-3

@xiaoy00
Copy link
Contributor Author

xiaoy00 commented Oct 22, 2019

/run agent-plugin-test-3

Copy link
Member

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

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Cast to approve as @zhaoyuguang approved.

@wu-sheng wu-sheng added this to the 6.5.0 milestone Oct 22, 2019
@wu-sheng wu-sheng merged commit 26b1d41 into apache:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants