Skip to content

Read the name and properties of the instance only in getInstance service.#10423

Closed
xzyJavaX wants to merge 3 commits intoapache:masterfrom
xzyJavaX:master
Closed

Read the name and properties of the instance only in getInstance service.#10423
xzyJavaX wants to merge 3 commits intoapache:masterfrom
xzyJavaX:master

Conversation

@xzyJavaX
Copy link
Copy Markdown
Contributor

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #10421
  • Update the CHANGES log.

@wu-sheng
Copy link
Copy Markdown
Member

If you want to add this, please make sure query dao would use this.

@wu-sheng wu-sheng added backend OAP backend related. enhancement Enhancement on performance or codes labels Feb 21, 2023
Comment thread docs/en/changes/changes.md Outdated
@wu-sheng wu-sheng changed the title Storage-elasticsearch-plugin supports query of selected fields. Read the name and properties of the instance only in getInstance service. Feb 24, 2023
@wu-sheng
Copy link
Copy Markdown
Member

@kezhenxu94 Please take a look.

Copy link
Copy Markdown
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.

Code level changes look good to me, but this may bring some troubles when future contributors use the getInstance and need the other fields like instance uuid, or id, they might have no idea why these two fields are null/empty

@wu-sheng
Copy link
Copy Markdown
Member

Code level changes look good to me, but this may bring some troubles when future contributors use the getInstance and need the other fields like instance uuid, or id, they might have no idea why these two fields are null/empty

@xzyJavaX You are better to keep consistent logic in JDBCMetadataQueryDAO`

Copy link
Copy Markdown
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.

I think it is wrong to build only two columns, even for BanyanDB.

@lujiajing1126 It seems BanyanDB implementation doesn't return required fields.

@wu-sheng wu-sheng closed this Mar 1, 2023
@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Mar 1, 2023

Close for now until we have a real use case to use Sources of ElasticSearch accessing.

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

Labels

backend OAP backend related. enhancement Enhancement on performance or codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants