[Refactor] OpenSearchException and ExceptionsHelper foundation to base class by nknize · Pull Request #7508 · opensearch-project/OpenSearch

nknize · 2023-05-10T18:37:28Z

This PR creates new BaseOpenSearchException and BaseExceptionsHelper classes in core library and refactors OpenSearchException and ExceptionsHelper foundation logic as a step to moving OpenSearch exception mechanisms from the :server module to :opensearch-core library. This is a move to support serverless and cloud native capabilities without requiring a dependency on the server module just for core opensearch exception mechanisms.

relates #5910

github-actions · 2023-05-10T18:56:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/15194/
CommitID: e8cdee5
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-05-11T19:41:25Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/15268/
CommitID: 1430a0a
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-05-11T19:49:15Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/15269/
CommitID: ba35fbd
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

dblock · 2023-05-11T21:35:18Z

Would it be better if we namespaced things right away and got rid of Base, meaning org.opensearch.BaseExceptionHelper was instead org.opensearch.core.ExceptionHelper?

nknize · 2023-05-12T14:51:56Z

Would it be better if we namespaced things right away and got rid of Base, meaning org.opensearch.BaseExceptionHelper was instead org.opensearch.core.ExceptionHelper?

The problem with that approach is not all of the logic can be rote refactored because of deep cross dependencies. Refactoring all of StreamInput / StreamOutput, OpenSearchException, for example, quickly spirals into force refactoring server specific classes we may not want in core (e.g., ClusterState). Separating into a Base / Concrete hierarchy enables us to thoughtfully refactor serverless logic up the hierarchy and control the blast radius.

Regarding OpenSearchException specifically, I think it would make sense to eventually introduce another mechanism called OpenSearchServerException to tease out more general index/search runtime exceptions from server specific faults. Then downstream serverless consumers only need take dependency on :libs:opensearch-core (and it's dependency libraries) and never need any of the cluster orchestration logic provided by :server

dbwiddis · 2023-05-19T00:26:49Z

@nknize this has a 2.8.0 label on it, should it be backported to 2.x?

monusingh-1 · 2023-05-19T05:52:17Z

@nknize , the other PR #7508 has label 2.x, will this change also go to 2.x ?

nknize · 2023-05-19T16:03:34Z

There are quite a few of these refactor PRs that I have labeled as 2.8.0. I haven't rushed to backport these yet, opting instead to let them bake in main. There's a lot more coming (see the goals outlined in #5910). So I wasn't planning to backport until, at minimum, OpenSearchException, Writeable, and StreamInput/Output settle down. Do y'all prefer these be backported now?

heemin32 · 2023-05-19T17:38:24Z

Prefer to backport it now if it will go in 2.8.0 release.

kotwanikunal · 2023-05-19T17:40:09Z

Prefer to backport it now if it will go in 2.8.0 release.

+1. It can bake in the meanwhile + give plugins some room to make their changes.

b4sjoo · 2023-05-19T19:17:29Z

I got following integ test error message after migration from org.opensearch.common.Strings to org.opensearch.core.common.Strings

»  java.lang.NoClassDefFoundError: org/opensearch/core/common/Strings
»  	at org.opensearch.ml.cluster.DiscoveryNodeHelper.<init>(DiscoveryNodeHelper.java:40)
»  	at org.opensearch.ml.plugin.MachineLearningPlugin.createComponents(MachineLearningPlugin.java:233)
»  	at org.opensearch.node.Node.lambda$new$16(Node.java:770)
»  	at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:273)
»  	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
»  	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
»  	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
»  	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
»  	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
»  	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
»  	at org.opensearch.node.Node.<init>(Node.java:784)
»  	at org.opensearch.node.Node.<init>(Node.java:377)
»  	at org.opensearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:242)
»  	at org.opensearch.bootstrap.Bootstrap.setup(Bootstrap.java:242)
»  	at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:404)
»  	at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:180)
»  	at org.opensearch.bootstrap.OpenSearch.execute(OpenSearch.java:171)
»  	at org.opensearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:104)
»  	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
»  	at org.opensearch.cli.Command.main(Command.java:101)
»  	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:137)
»  	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:103)

The line for DiscoveryNodeHelper.java:40 is

excludedNodeNames = Strings.commaDelimitedListToSet(ML_COMMONS_EXCLUDE_NODE_NAMES.get(settings));

And UnitTests were passed. Any thoughts?

dbwiddis · 2023-05-19T19:19:51Z

+1 here too. At SDK we maintain a 2.x compatible and 3.x compatible branch. Leaving fixes open on 2.x is doable but annoying.

heemin32 · 2023-05-19T19:30:04Z

In my understanding, integ test is failing because integ test use release artifacts to launch a cluster and this change is not released to the repository yet.

…e class (opensearch-project#7508) * [Refactor] OpenSearchException and ExceptionsHelper foundation to base class Creates new BaseOpenSearchException and BaseExceptionsHelper in core library as a step to moving OpenSearch exception mechanisms from the server module to core library. This is a move to support serverless and cloud native capabilities without requiring the server module for core opensearch indexing, search, and compute capabilities. Signed-off-by: Nicholas Walter Knize <nknize@apache.org> Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

…e class (opensearch-project#7508) * [Refactor] OpenSearchException and ExceptionsHelper foundation to base class Creates new BaseOpenSearchException and BaseExceptionsHelper in core library as a step to moving OpenSearch exception mechanisms from the server module to core library. This is a move to support serverless and cloud native capabilities without requiring the server module for core opensearch indexing, search, and compute capabilities. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

opensearch-trigger-bot · 2023-07-05T23:26:23Z

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-7508-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1e08b5a075fa6018be6c0af959b2d638c2743d56
# Push it to GitHub
git push --set-upstream origin backport/backport-7508-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-7508-to-2.x.

…e class (opensearch-project#7508) Creates new BaseOpenSearchException and BaseExceptionsHelper in core library as a step to moving OpenSearch exception mechanisms from the server module to core library. This is a move to support serverless and cloud native capabilities without requiring the server module for core opensearch indexing, search, and compute capabilities. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

…e class (#7508) (#8460) Creates new BaseOpenSearchException and BaseExceptionsHelper in core library as a step to moving OpenSearch exception mechanisms from the server module to core library. This is a move to support serverless and cloud native capabilities without requiring the server module for core opensearch indexing, search, and compute capabilities. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

lukas-vlcek · 2023-07-31T11:12:03Z

Hi,

I just wanted to point out that this change was a breaking change and should have been in change log.
As and author of (external) plugin I was using the Strings class and moving it to a different module/package was a breaking change from my POW.

Just my 2 cents,
Lukáš

The method to split a string was moved to a different module/package. See the following ticket for more details: <opensearch-project/OpenSearch#7508> Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>

…e class (opensearch-project#7508) * [Refactor] OpenSearchException and ExceptionsHelper foundation to base class Creates new BaseOpenSearchException and BaseExceptionsHelper in core library as a step to moving OpenSearch exception mechanisms from the server module to core library. This is a move to support serverless and cloud native capabilities without requiring the server module for core opensearch indexing, search, and compute capabilities. Signed-off-by: Nicholas Walter Knize <nknize@apache.org> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

nknize requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, dblock, dreamer-89, gbbafna, kartg, kotwanikunal, mch2, owaiskazi19, reta, ryanbogan, saratvemulapalli, setiah, shwetathareja and tlfeng as code owners May 10, 2023 18:37

nknize added enhancement Enhancement or improvement to existing feature or request skip-changelog v2.8.0 'Issues and PRs related to version v2.8.0' labels May 10, 2023

nknize force-pushed the opensearchException-base branch from 1430a0a to ba35fbd Compare May 11, 2023 19:21

nknize force-pushed the opensearchException-base branch 2 times, most recently from 06b463d to a754c94 Compare May 16, 2023 14:47

martin-gaievski mentioned this pull request May 19, 2023

Fix build errors after refactoring of Strings class in core opensearch-project/k-NN#904

Merged

2 tasks

This was referenced May 19, 2023

Fix build errors after refactoring of Strings class in core opensearch-project/index-management#786

Merged

Fix build errors after refactoring of Strings class in core opensearch-project/common-utils#432

Merged

joshpalis mentioned this pull request May 19, 2023

[Backport 1.x] [Extensions] Fixing Imports String.java opensearch-project/opensearch-sdk-java#765

Merged

DarshitChanpura mentioned this pull request May 19, 2023

Fixes LoggerMessageFormat import to use correct package in core opensearch-project/security#2783

Closed

1 task

MaxKsyunz mentioned this pull request May 19, 2023

Keep up with refactors in core -- Strings class opensearch-project/sql#1645

Merged

martin-gaievski mentioned this pull request May 24, 2023

[BUG] Artifacts are not correctly pulled by integ test distribution #7714

Closed

nknize added backport 2.x Backport to 2.x branch v2.9.0 'Issues and PRs related to version v2.9.0' and removed v2.8.0 'Issues and PRs related to version v2.8.0' labels Jul 5, 2023

nknize mentioned this pull request Jul 6, 2023

[Backport 2.x] [Refactor] OpenSearchException and ExceptionsHelper foundation to base class (#7508) #8460

Merged

lukas-vlcek mentioned this pull request Aug 1, 2023

[Search Pipelines] Add stats for search pipelines #8053

Merged

6 tasks

saratvemulapalli mentioned this pull request Aug 7, 2023

[2.x] Fixing breaking changes from core opensearch-project/ml-commons#1189

Closed

5 tasks

Conversation

nknize commented May 10, 2023

Uh oh!

github-actions bot commented May 10, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 11, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 11, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

dblock commented May 11, 2023

Uh oh!

nknize commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbwiddis commented May 19, 2023

Uh oh!

monusingh-1 commented May 19, 2023

Uh oh!

nknize commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heemin32 commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotwanikunal commented May 19, 2023

Uh oh!

b4sjoo commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbwiddis commented May 19, 2023

Uh oh!

heemin32 commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opensearch-trigger-bot bot commented Jul 5, 2023

Uh oh!

lukas-vlcek commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nknize commented May 12, 2023 •

edited

Loading

nknize commented May 19, 2023 •

edited

Loading

heemin32 commented May 19, 2023 •

edited

Loading

b4sjoo commented May 19, 2023 •

edited

Loading

heemin32 commented May 19, 2023 •

edited

Loading