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

Optimize Node, remove duplicate Settings #2703

Merged
merged 2 commits into from
Apr 4, 2022
Merged

Optimize Node, remove duplicate Settings #2703

merged 2 commits into from
Apr 4, 2022

Conversation

ruanwenjun
Copy link
Contributor

Description

I find some code might be optimized.

Issues Resolved

No related issue, since this is a small optimize

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ruanwenjun
Optimize Node, remove duplicate Settings
bbbfdc3 
Signed-off-by: ruanwenjun <wenjun@apache.org>
@ruanwenjun ruanwenjun requested a review from a team as a code owner April 1, 2022 11:56
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure bbbfdc3
Log 4028

Reports 4028

@tlfeng tlfeng added v2.1.0 Issues and PRs related to version 2.1.0 v3.0.0 Issues and PRs related to version 3.0.0 backport 2.x Backport to 2.x branch enhancement Enhancement or improvement to existing feature or request non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues labels Apr 2, 2022
tlfeng
tlfeng reviewed Apr 2, 2022
View reviewed changes
Copy link
Collaborator

@tlfeng tlfeng left a comment

Choose a reason for hiding this comment

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

You are so careful that find out these codes to be refactored! 👍👍
In additon, please run ./gradlew spotlessApply to formatting your code, which is shown in the Gradle check log. 😁

server/src/main/java/org/opensearch/index/get/ShardGetService.java Outdated

try {
)) {
if (get.exists() == false) {
Copy link
Collaborator

@tlfeng tlfeng Apr 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A small question, does it cover the condition when get == null?
(For reference: here is the GetResult class)

Copy link
Contributor Author

@ruanwenjun ruanwenjun Apr 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@tlfeng Thanks for your review, I checked, the GetResult will never be null here, but this is a good suggestion, add a check is better.
And I formatted the code, please review, thanks.

Copy link
Collaborator

@tlfeng tlfeng Apr 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your time checking the situation! I'm just curious, because I'm not familiar with this part of code, not exactly a suggestion 😄.
And a note, every commit has to be signed off 😉 git commit -s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, added the signed of :).

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 68e49931dc1faa21669fb8b43f05d9742e19060c
Log 4066

Reports 4066

@ruanwenjun
fix code style
b5762de 
Signed-off-by: ruanwenjun <wenjun@apache.org>
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_cleanCode branch from 68e4993 to b5762de Compare April 2, 2022 05:59
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success b5762de
Log 4067

Reports 4067

@ruanwenjun ruanwenjun requested a review from tlfeng April 2, 2022 15:38
tlfeng
tlfeng approved these changes Apr 2, 2022
View reviewed changes
Copy link
Collaborator

@tlfeng tlfeng left a comment

Choose a reason for hiding this comment

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

Thanks for making the code concise! 😄

@ruanwenjun
Copy link
Contributor Author

Thanks for making the code concise! 😄

You are welcome, I am interested in OpenSearch, hope to make this project better.

nknize
nknize approved these changes Apr 4, 2022
View reviewed changes
Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

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

Thx for the cleanup!

@nknize nknize merged commit d848c63 into opensearch-project:main Apr 4, 2022
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Apr 4, 2022
Merged
nknize pushed a commit that referenced this pull request Apr 4, 2022
@ruanwenjun @nknize
Optimize Node, remove duplicate Settings (#2703)
54aab86 
Removes duplicate Settings

Signed-off-by: ruanwenjun <wenjun@apache.org>
Signed-off-by: Nicholas Walter knize <nknize@apache.org>
(cherry picked from commit d848c63)
nknize pushed a commit that referenced this pull request Apr 4, 2022
@opensearch-trigger-bot @ruanwenjun
Optimize Node, remove duplicate Settings (#2703) (#2742)
1884de5 
Removes duplicate Settings

Signed-off-by: ruanwenjun <wenjun@apache.org>
Signed-off-by: Nicholas Walter knize <nknize@apache.org>
(cherry picked from commit d848c63)

Co-authored-by: Wenjun Ruan <wenjun@apache.org>
@ruanwenjun ruanwenjun deleted the dev_wenjun_cleanCode branch April 5, 2022 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nknize nknize nknize approved these changes

@tlfeng tlfeng tlfeng approved these changes

Assignees
No one assigned
Labels
backport 2.x Backport to 2.x branch enhancement Enhancement or improvement to existing feature or request non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues v2.1.0 Issues and PRs related to version 2.1.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ruanwenjun @opensearch-ci-bot @nknize @tlfeng