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

support Nacos cluster #2640

Merged
merged 37 commits into from
May 14, 2019
Merged

support Nacos cluster #2640

merged 37 commits into from
May 14, 2019

Conversation

IanCao
Copy link
Contributor

@IanCao IanCao commented May 9, 2019

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues


Bug fix

  • Bug description.

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.

@IanCao IanCao requested a review from wu-sheng May 9, 2019 08:27
@wu-sheng
Copy link
Member

wu-sheng commented May 9, 2019

Please check Nacos license issue first, then we could talk about this PR.

@IanCao
Copy link
Contributor Author

IanCao commented May 10, 2019

There is something missing in the screenshot, could you provide the difference between before and after the nacos dependency? Which libs version update? Which libs are added?

the check result: five jars added, no jar's version update.
the five jar list:

  1. cluster-nacos-plugin-6.2.0-SNAPSHOT.jar
  2. fastjson-1.2.47.jar
  3. nacos-api-1.0.0.jar
  4. nacos-client-1.0.0.jar
  5. nacos-common-1.0.0.jar

@IanCao
Copy link
Contributor Author

IanCao commented May 11, 2019

i have updated the license, plz review

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.

SkyWalking dist LICENSE file has not been updated.


===========================================================================
fastjson-1.2.47 Notice
===========================================================================
Copy link
Member

Choose a reason for hiding this comment

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

fastjson notice should be removed. It doesn't include more things than its license file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

apm-dist/release-docs/licenses/LICENSE-nacos.txt Outdated Show resolved Hide resolved
docs/en/setup/backend/backend-cluster.md Show resolved Hide resolved
@wu-sheng wu-sheng added feature New feature backend OAP backend related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. and removed TBD To be decided later, need more discussion or input. labels May 12, 2019
@wu-sheng wu-sheng added this to the 6.2.0 milestone May 12, 2019
@wu-sheng
Copy link
Member

@peng-yongsheng Please recheck the document/LICENSE first, the new Nacos version with the exclusions should be good.

@peng-yongsheng
Copy link
Member

Hi @IanCao, can you help me to correct some mistakes about not clear dependency management. I checked the dependency tree. And I found those third-party libraries are not managed by the OAP module's dependency manager, etc.. jackson-core, jackson-annotations, jackson-databind, commons-codec, commons-lang3.
This is not caused by you. Those libraries are not managed before. ^_^

@wu-sheng
Copy link
Member

@peng-yongsheng could we change those through different pull request? Look like irrelevant.

@peng-yongsheng
Copy link
Member

image

Image source

From this compile dependencies list, his exclude list is right and the third-party library's licenses are no problem. @wu-sheng

@peng-yongsheng
Copy link
Member

@peng-yongsheng could we change those through different pull request? Look like irrelevant.

Those third-party libraries are using by gPRC and zookepper, but with different version. I'm not sure the OAP server is work well after the version changed.

@peng-yongsheng
Copy link
Member

@peng-yongsheng could we change those through different pull request? Look like irrelevant.

@wu-sheng I think keeping those third-party libraries version is the best solution. He can only test the nacos client without test the whole OAP server. So, he can find the version number from current master source then put them into the dependency management.

Copy link
Member

@peng-yongsheng peng-yongsheng left a comment

Choose a reason for hiding this comment

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

Waiting for dependency management.

@wu-sheng
Copy link
Member

@peng-yongsheng I am just suggesting, we could try to work on another pull request to make sure the dependency clear for current master, then we update this pr.

@IanCao
Copy link
Contributor Author

IanCao commented May 14, 2019

Hi @IanCao, can you help me to correct some mistakes about not clear dependency management. I checked the dependency tree. And I found those third-party libraries are not managed by the OAP module's dependency manager, etc.. jackson-core, jackson-annotations, jackson-databind, commons-codec, commons-lang3.
This is not caused by you. Those libraries are not managed before. ^_^

ok, i'll make the dependency management more clear and open a new pr to fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend 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

5 participants