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

[test/plugin] immigrate test hystrix scenario. #3887

Merged
merged 7 commits into from Nov 19, 2019
Merged

[test/plugin] immigrate test hystrix scenario. #3887

merged 7 commits into from Nov 19, 2019

Conversation

ghost
Copy link

@ghost ghost commented Nov 18, 2019

Please answer these questions before submitting pull request


Bug fix

  • Bug description.

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.

@dmsolr dmsolr added plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. test Test requirements about performance, feature or before release. labels Nov 18, 2019
@dmsolr dmsolr added this to the 6.6.0 milestone Nov 18, 2019
scenario name | versions | elapsed time (sec)
---|---|---
hystrix-scenario | 20 | 799.00
Copy link
Member

Choose a reason for hiding this comment

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

place it in Workload1 Group2. Because the group1 also takes almost 3000s.

Copy link
Author

Choose a reason for hiding this comment

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

Before I modified, According to Plugin-test.md, workload3 group1 is little faster than workload1 Group 2

Copy link
Member

Choose a reason for hiding this comment

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

I see. But a workload is waiting for both groups to finish, two groups of workload take the same time will be better.

Copy link
Author

Choose a reason for hiding this comment

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

if add to workload1 group2,total is:3090.98. current workload3 group1, total is: 3090.912, is right?

Copy link
Member

Choose a reason for hiding this comment

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

@Aderm According to document, workload1 group2 is only 2300s. Much faster than 3000. Where is your data from?

image

Also verified from the CI process, https://builds.apache.org/blue/organizations/jenkins/skywalking-agent-test/detail/skywalking-agent-test/1297/pipeline/135

Copy link
Author

Choose a reason for hiding this comment

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

I mean :
current: workload1 group2(2291.98s)
current: workload3 group1(2291.912s)
If you follow the fasterer, you should choose workload3 group1

799 is hystrix-scenario time
3090.98= 2291.98+799 (if put on workload1 group2)
3090.912= 2291.912+799 (if put on workload3 group1)
The total is as a comparison, there is no other meaning

Copy link
Member

Choose a reason for hiding this comment

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

OK, make sense. I am good.


@Override
protected String run() throws Exception {
Thread.sleep(2001);
Copy link
Member

Choose a reason for hiding this comment

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

Usually, I don't recommend to do that. If have to do, I think it doesn't over 1000ms. (Personal suggestion)

Copy link
Member

Choose a reason for hiding this comment

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

Because it is easy to cause by timeout. WDYT, @arugal

Copy link
Author

Choose a reason for hiding this comment

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

Maybe understand a little bit different, this logic is to trigger the timeout fuse, now the fuse time is 1s

Copy link
Member

Choose a reason for hiding this comment

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

I mean that the request(/hystrix-scenario) timeout so that test failures. before Hystrix works. So that we will get the unexpected data.

Copy link
Author

Choose a reason for hiding this comment

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

This setting does not affect, although sleep 2s in run method, the front end rest request hystrix will returns fallback result when run over 1s.

Copy link
Member

Choose a reason for hiding this comment

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

Then why sleep 2s?

Copy link
Member

Choose a reason for hiding this comment

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

For triggering fusing. @wu-sheng

Copy link
Member

Choose a reason for hiding this comment

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

OK, if fusing in the case, then make sense.

Copy link
Author

Choose a reason for hiding this comment

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

This is because there is a hystrix blown test scene.

@dmsolr dmsolr added the agent Language agent related. label Nov 18, 2019
@wu-sheng wu-sheng requested a review from dmsolr November 19, 2019 06:12
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.

LGTM. @dmsolr Please confirm.

Copy link
Member

@dmsolr dmsolr left a comment

Choose a reason for hiding this comment

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

LGTM

@wu-sheng wu-sheng merged commit a2dce71 into apache:master Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. test Test requirements about performance, feature or before release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants