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

[CCI] [BUG] Fixing extension settings update consumers #7456

Merged
merged 8 commits into from
Jun 21, 2023

Conversation

Kuanysh-kst
Copy link
Contributor

@Kuanysh-kst Kuanysh-kst commented May 7, 2023

Description

added equals to the method to remove the bug, which appears in the addSettingsUpdateConsumer() method

Related Issues

opensearch-project/opensearch-sdk-java#366

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
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2023

Gradle Check (Jenkins) Run Completed with:

@dblock
Copy link
Member

dblock commented May 8, 2023

@Kuanysh-kst Thanks. Care to describe what you're fixing? Add a unit test please.

Signed-off-by: Kuanysh <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@kotwanikunal kotwanikunal added CCI College Contributor Initiative related issues and PRs skip-changelog labels May 11, 2023
Signed-off-by: Kuanysh <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@Kuanysh-kst Kuanysh-kst requested a review from sachinpkale as a code owner June 21, 2023 05:45
Signed-off-by: Kuanysh Aimurzinov <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@Kuanysh-kst Kuanysh-kst requested a review from dbwiddis June 21, 2023 15:30
@dbwiddis
Copy link
Member

Gradle check failure unrelated to this PR:

  • What went wrong:
    Execution failed for task ':distribution:bwc:minor:buildBwcLinuxTar'.

Building 2.8.0 didn't generate expected file /var/jenkins/workspace/gradle-check/search/distribution/bwc/minor/build/bwc/checkout-2.x/distribution/archives/linux-tar/build/distributions/opensearch-min-2.8.0-SNAPSHOT-linux-x64.tar.gz

@dbwiddis
Copy link
Member

Looks similar to #7396

@Kuanysh-kst can you try to rebase your branch? I think that'll fix the gradle check.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dbwiddis
Copy link
Member

dbwiddis commented Jun 21, 2023

@Kuanysh-kst Good news, tests are running. Bad news, test failure:

java.lang.AssertionError: expected:<false> but was:<true>
at org.opensearch.extensions.ExtensionsManagerTests.testHandleAddSettingsUpdateConsumerRequest(ExtensionsManagerTests.java:785)

That's this line:

        // Should fail as component settings are not registered within cluster settings
        assertEquals(false, ((AcknowledgedResponse) response).getStatus());

The code changes status to false on exception, but doesn't do anything in the case that the setting is not registered (node or index scope) which this PR has turned into a no-op.

This seems correlated with this comment saying we should do a null check (and we don't):

// do a null check and throw IllegalArgument exception here if neither index or node scope

Suggested fix:

  1. Do the null check on settingForUpdateConsumer right below that comment line.
  2. Throw an illegal argument exception stating the reason for the exception (not index or node scope)
  3. Add IllegalArgumentException as one of the caught exceptions in the catch block in addition to the SettingsException

@dbwiddis dbwiddis self-requested a review June 21, 2023 19:36
@dbwiddis dbwiddis dismissed their stale review June 21, 2023 19:36

Need to fix tests

Signed-off-by: Kuanysh <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests.testBasicTaskResourceTracking

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #7456 (fc3cda7) into main (68c1c86) will decrease coverage by 0.01%.
The diff coverage is 19.04%.

@@             Coverage Diff              @@
##               main    #7456      +/-   ##
============================================
- Coverage     70.93%   70.93%   -0.01%     
+ Complexity    56636    56614      -22     
============================================
  Files          4719     4719              
  Lines        267559   267574      +15     
  Branches      39206    39211       +5     
============================================
- Hits         189805   189798       -7     
- Misses        61714    61730      +16     
- Partials      16040    16046       +6     
Impacted Files Coverage Δ
...a/org/opensearch/extensions/ExtensionsManager.java 57.07% <ø> (ø)
...sions/AddSettingsUpdateConsumerRequestHandler.java 44.73% <19.04%> (-20.49%) ⬇️

... and 489 files with indirect coverage changes

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Added opensearch-project/opensearch-sdk-java#835 to follow up with SDK integration tests for the lines called out by codecov.

@dbwiddis dbwiddis changed the title [CCI] [BUG] Fixing an error related to Setting [CCI] [BUG] Fixing extension settings update consumers Jun 21, 2023
@owaiskazi19 owaiskazi19 added the backport 2.x Backport to 2.x branch label Jun 21, 2023
@owaiskazi19 owaiskazi19 merged commit cc67469 into opensearch-project:main Jun 21, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7456-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cc67469292bc8d5be1b476fb0fb11e07fe82f8a6
# Push it to GitHub
git push --set-upstream origin backport/backport-7456-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7456-to-2.x.

gaiksaya pushed a commit to gaiksaya/OpenSearch that referenced this pull request Jun 21, 2023
…oject#7456)

* add equels for ClusterSettings

Signed-off-by: Kuanysh <[email protected]>

* added junit

Signed-off-by: Kuanysh <[email protected]>

* code refactoring

Signed-off-by: Kuanysh <[email protected]>

* added changes to handleAddSettingsUpdateConsumer

Signed-off-by: Kuanysh <[email protected]>

* code refactoring

Signed-off-by: Kuanysh Aimurzinov <[email protected]>

* code refactoring

Signed-off-by: Kuanysh Aimurzinov <[email protected]>

* changed main method

Signed-off-by: Kuanysh <[email protected]>

---------

Signed-off-by: Kuanysh <[email protected]>
Signed-off-by: Kuanysh <[email protected]>
Signed-off-by: Kuanysh Aimurzinov <[email protected]>
imRishN pushed a commit to imRishN/OpenSearch that referenced this pull request Jun 27, 2023
…oject#7456)

* add equels for ClusterSettings

Signed-off-by: Kuanysh <[email protected]>

* added junit

Signed-off-by: Kuanysh <[email protected]>

* code refactoring

Signed-off-by: Kuanysh <[email protected]>

* added changes to handleAddSettingsUpdateConsumer

Signed-off-by: Kuanysh <[email protected]>

* code refactoring

Signed-off-by: Kuanysh Aimurzinov <[email protected]>

* code refactoring

Signed-off-by: Kuanysh Aimurzinov <[email protected]>

* changed main method

Signed-off-by: Kuanysh <[email protected]>

---------

Signed-off-by: Kuanysh <[email protected]>
Signed-off-by: Kuanysh <[email protected]>
Signed-off-by: Kuanysh Aimurzinov <[email protected]>
Signed-off-by: Rishab Nahata <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…oject#7456)

* add equels for ClusterSettings

Signed-off-by: Kuanysh <[email protected]>

* added junit

Signed-off-by: Kuanysh <[email protected]>

* code refactoring

Signed-off-by: Kuanysh <[email protected]>

* added changes to handleAddSettingsUpdateConsumer

Signed-off-by: Kuanysh <[email protected]>

* code refactoring

Signed-off-by: Kuanysh Aimurzinov <[email protected]>

* code refactoring

Signed-off-by: Kuanysh Aimurzinov <[email protected]>

* changed main method

Signed-off-by: Kuanysh <[email protected]>

---------

Signed-off-by: Kuanysh <[email protected]>
Signed-off-by: Kuanysh <[email protected]>
Signed-off-by: Kuanysh Aimurzinov <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch CCI College Contributor Initiative related issues and PRs skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants