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.

Sign up for GitHub

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

Jump to bottom

Provide plugin for Ehcache 2.x #3575

Merged
merged 26 commits into from
Oct 10, 2019
Merged

Provide plugin for Ehcache 2.x #3575

merged 26 commits into from
Oct 10, 2019

Conversation

mrproliu
Copy link
Contributor

@mrproliu mrproliu commented Oct 8, 2019

Please answer these questions before submitting pull request

@wu-sheng
Copy link
Member

wu-sheng commented Oct 8, 2019

Make ci and e2e passed please. And after #3528 merged, plugin test is moving to PR process rather than in separated repo.

@wu-sheng wu-sheng added agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. TBD To be decided later, need more discussion or input. labels Oct 8, 2019
wu-sheng
wu-sheng reviewed Oct 9, 2019
View reviewed changes
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.

  1. Supported-list document needs to be updated
  2. Plugin test case required(Known that, you are WIP)

@wu-sheng
Copy link
Member

wu-sheng commented Oct 9, 2019

You need to fix the header issue. We expected header including Apache 2.0. Please run the command locally and fix the files.

@wu-sheng wu-sheng added feature New feature and removed TBD To be decided later, need more discussion or input. labels Oct 9, 2019
@wu-sheng
Copy link
Member

wu-sheng commented Oct 9, 2019 •
edited
Loading

Read Jenkinsfile-Agent-Test file, you need to add a stage called Ehcache 2.x. Then the tests are included in PR process.

wu-sheng
wu-sheng reviewed Oct 9, 2019
View reviewed changes
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.

You need Unit tests for your intercepters(don't need for instrumentation definitions).

oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/pom.xml Outdated Show resolved Hide resolved
@wu-sheng
Copy link
Member

wu-sheng commented Oct 9, 2019

@dmsolr Ehcache plugin tests take 7m43s

@wu-sheng wu-sheng added this to the 6.5.0 milestone Oct 9, 2019
@dmsolr
Copy link
Member

dmsolr commented Oct 9, 2019

@dmsolr Ehcache plugin tests take 7m43s

there are 19 cases. It took so long time.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 9, 2019

there are 19 cases. It took so long time.

Make sense to me.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 9, 2019

You are using Chinese comment. Please don't.

kezhenxu94
kezhenxu94 reviewed Oct 9, 2019
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

inline

...etwork/src/main/java/org/apache/skywalking/apm/network/trace/component/ComponentsDefine.java Outdated Show resolved Hide resolved
docs/en/setup/service-agent/java-agent/Supported-list.md Show resolved Hide resolved
@kezhenxu94 kezhenxu94 mentioned this pull request Oct 9, 2019
Merged
3 tasks
@wu-sheng wu-sheng mentioned this pull request Oct 9, 2019
Closed
wu-sheng
wu-sheng approved these changes Oct 10, 2019
View reviewed changes
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. @kezhenxu94 @dmsolr @zhaoyuguang Please recheck and confirm.

This was referenced Oct 10, 2019
Closed
Merged
kezhenxu94
kezhenxu94 approved these changes Oct 10, 2019
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

LGTM.

@mrproliu then please submit another PR to add the logo/icon for Ehcache.

https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets/

@wu-sheng
Copy link
Member

then please submit another PR to add the logo/icon for Ehcache.

@kezhenxu94 As a local span only, I think we don't need that.

@wu-sheng wu-sheng merged commit 5880417 into apache:master Oct 10, 2019
@kezhenxu94
Copy link
Member

then please submit another PR to add the logo/icon for Ehcache.

@kezhenxu94 As a local span only, I think we don't need that.

Ooops, I missed that, forgive me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmsolr dmsolr dmsolr left review comments

@wu-sheng wu-sheng wu-sheng approved these changes

@kezhenxu94 kezhenxu94 kezhenxu94 approved these changes

@zhaoyuguang zhaoyuguang Awaiting requested review from zhaoyuguang

Assignees
No one assigned
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
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mrproliu @wu-sheng @dmsolr @kezhenxu94