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

add graphql getServiceInstance instanceUUID field. #3595

Merged
merged 14 commits into from
Oct 17, 2019
Merged

add graphql getServiceInstance instanceUUID field. #3595

merged 14 commits into from
Oct 17, 2019

Conversation

x22x22
Copy link
Member

@x22x22 x22x22 commented Oct 10, 2019 •
edited
Loading

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.

add graphql getServiceInstance instanceUUID field.

apache/skywalking-query-protocol#18

image

@x22x22 x22x22 mentioned this pull request Oct 10, 2019
Merged
@x22x22 x22x22 requested a review from wu-sheng October 10, 2019 17:19
@x22x22 x22x22 added this to the 6.5.0 milestone Oct 10, 2019
wu-sheng
wu-sheng requested changes Oct 11, 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.

Why add this? We don't have this field in GraphQL definition.

And you just change one implementation of storage.

@wu-sheng
Copy link
Member

The graphQL definition is like this

type ServiceInstance {
    id: ID!
    name: String!
    attributes: [Attribute!]!
    language: Language!
}

@x22x22 x22x22 changed the title add graphql getServiceInstance instanceUUID field. [WIP]add graphql getServiceInstance instanceUUID field. Oct 11, 2019
@x22x22
Copy link
Member Author

x22x22 commented Oct 11, 2019

Why add this? We don't have this field in GraphQL definition.

And you just change one implementation of storage.

  1. add_graphql_ServiceInstance_instanceUUID_field skywalking-query-protocol#18
  2. sorry, I will wait to change it.

@wu-sheng wu-sheng added the TBD To be decided later, need more discussion or input. label Oct 11, 2019
@x22x22 x22x22 changed the title [WIP]add graphql getServiceInstance instanceUUID field. add graphql getServiceInstance instanceUUID field. Oct 13, 2019
@x22x22
Copy link
Member Author

x22x22 commented Oct 13, 2019 •
edited
Loading

@wu-sheng

  1. H2 has added the relevant code of the instanceUUID field.
  2. e2e has added the relevant code of the instanceUUID field.

@wu-sheng wu-sheng added feature New feature backend OAP backend related. and removed TBD To be decided later, need more discussion or input. labels Oct 14, 2019
kezhenxu94
kezhenxu94 reviewed Oct 14, 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.

@x22x22 add it to org.apache.skywalking.e2e.service.instance.InstanceMatcher and at least add a non-null assertion to expected-data of each test case please.

@wu-sheng
Copy link
Member

@x22x22 Update?

@wu-sheng wu-sheng removed this from the 6.5.0 milestone Oct 16, 2019
@x22x22
Copy link
Member Author

x22x22 commented Oct 16, 2019

@x22x22 add it to org.apache.skywalking.e2e.service.instance.InstanceMatcher and at least add a non-null assertion to expected-data of each test case please.

@wu-sheng @kezhenxu94

done.

@wu-sheng wu-sheng added this to the 6.5.0 milestone Oct 16, 2019
wu-sheng
wu-sheng reviewed Oct 16, 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 Please confirm the e2e test changes.

@wu-sheng wu-sheng merged commit c0be757 into apache:master Oct 17, 2019
@x22x22 x22x22 deleted the add_graphql_getServiceInstance_instanceUUID_field branch November 3, 2019 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@kezhenxu94 kezhenxu94 kezhenxu94 approved these changes

Assignees
No one assigned
Labels
backend OAP backend related. feature New feature
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@x22x22 @wu-sheng @kezhenxu94