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

[Refactor] Various Utilities, Assertion, and Concurrency Exception from server to libraries #6875

Merged
merged 4 commits into from
Apr 3, 2023
Merged

[Refactor] Various Utilities, Assertion, and Concurrency Exception from server to libraries #6875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d89e8d4
[Refactor] Various Utilities, Assertion, and Concurrency Exception fr…
nknize Mar 29, 2023
8d6af82
remove dependency on opensearch-core lib from opensearch-nio
nknize Mar 31, 2023
12d20bd
remove opensearch-core dependency from ssl-config and nio libraries
nknize Mar 31, 2023
138b528
remove opensearch-cli dependency on opensearch-core
nknize Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...high-level/src/test/java/org/opensearch/client/OpenSearchRestHighLevelClientTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
import org.opensearch.common.xcontent.XContentHelper;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.common.xcontent.json.JsonXContent;
import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;
import org.opensearch.ingest.Pipeline;
import org.opensearch.search.SearchHit;
import org.opensearch.search.SearchModule;
Expand Down
2 changes: 1 addition & 1 deletion distribution/tools/keystore-cli/src/test/java/org/opensearch/bootstrap/BootstrapTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

package org.opensearch.bootstrap;

import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;
import org.opensearch.common.settings.KeyStoreCommandTestCase;
import org.opensearch.common.settings.KeyStoreWrapper;
import org.opensearch.common.settings.SecureSettings;
Expand Down
2 changes: 1 addition & 1 deletion ...ls/keystore-cli/src/test/java/org/opensearch/common/settings/KeyStoreCommandTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@

import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;
import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;
import org.apache.lucene.tests.util.LuceneTestCase;
import org.opensearch.cli.CommandTestCase;
import org.opensearch.common.io.PathUtilsForTesting;
Expand Down
2 changes: 1 addition & 1 deletion ...tools/keystore-cli/src/test/java/org/opensearch/common/settings/KeyStoreWrapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
import org.apache.lucene.store.IndexOutput;
import org.apache.lucene.store.NIOFSDirectory;
import org.opensearch.common.Randomness;
import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;
import org.opensearch.env.Environment;
import org.opensearch.test.OpenSearchTestCase;
import org.hamcrest.Matchers;
Expand Down
2 changes: 1 addition & 1 deletion distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
import org.opensearch.common.SuppressForbidden;
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.hash.MessageDigests;
import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;
import org.opensearch.env.Environment;

import java.io.BufferedReader;
Expand Down
2 changes: 1 addition & 1 deletion distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/PluginCli.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

package org.opensearch.plugins;

import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;
import org.opensearch.cli.Command;
import org.opensearch.cli.LoggingAwareMultiCommand;
import org.opensearch.cli.Terminal;
Expand Down
2 changes: 1 addition & 1 deletion distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/RemovePluginCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
import org.opensearch.cli.ExitCodes;
import org.opensearch.cli.Terminal;
import org.opensearch.cli.UserException;
import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;
import org.opensearch.env.Environment;

import java.io.IOException;
Expand Down
2 changes: 1 addition & 1 deletion distribution/tools/upgrade-cli/src/test/java/org/opensearch/upgrade/UpgradeCliTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import org.opensearch.common.io.PathUtilsForTesting;
import org.opensearch.common.settings.KeyStoreWrapper;
import org.opensearch.common.settings.Settings;
import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;
import org.opensearch.env.Environment;
import org.opensearch.env.TestEnvironment;

Expand Down
1 change: 0 additions & 1 deletion libs/cli/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ apply plugin: 'opensearch.publish'
dependencies {
api 'net.sf.jopt-simple:jopt-simple:5.0.4'
api project(':libs:opensearch-common')
api project(':libs:opensearch-core')
}

test.enabled = false
Expand Down
2 changes: 1 addition & 1 deletion libs/cli/src/main/java/org/opensearch/cli/MultiCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
import joptsimple.OptionSet;
import joptsimple.OptionSpec;
import joptsimple.util.KeyValuePair;
import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.common.util.io.IOUtils;

import java.io.IOException;
import java.util.ArrayList;
Expand Down
0 ...opensearch/common/collect/MapBuilder.java → ...opensearch/common/collect/MapBuilder.java
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion .../opensearch/core/internal/io/IOUtils.java → ...rg/opensearch/common/util/io/IOUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
* GitHub history for details.
*/

package org.opensearch.core.internal.io;
package org.opensearch.common.util.io;

import org.opensearch.common.Nullable;

Expand Down
2 changes: 1 addition & 1 deletion .../opensearch/core/internal/io/Streams.java → ...rg/opensearch/common/util/io/Streams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
* GitHub history for details.
*/

package org.opensearch.core.internal.io;
package org.opensearch.common.util.io;

import java.io.IOException;
import java.io.InputStream;
Expand Down
4 changes: 2 additions & 2 deletions ...search/core/internal/io/package-info.java → ...ensearch/common/util/io/package-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@
* compatible open source license.
*/

/** Common internal I/O classes */
package org.opensearch.core.internal.io;
/** Common internal I/O utility classes */
package org.opensearch.common.util.io;
2 changes: 1 addition & 1 deletion ...pensearch/core/internal/net/NetUtils.java → .../opensearch/common/util/net/NetUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
* GitHub history for details.
*/

package org.opensearch.core.internal.net;
package org.opensearch.common.util.net;

import java.io.IOException;
import java.lang.reflect.Field;
Expand Down
2 changes: 1 addition & 1 deletion ...earch/core/internal/net/package-info.java → ...nsearch/common/util/net/package-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
*/

/** Common Network Utility Classes */
package org.opensearch.core.internal.net;
package org.opensearch.common.util.net;
4 changes: 4 additions & 0 deletions libs/core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ dependencies {

api "com.fasterxml.jackson.core:jackson-core:${versions.jackson}"

// lucene
api "org.apache.lucene:lucene-core:${versions.lucene}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nknize I have doubts we need to depend on Lucene in any core / common modules, the BytesRefUtils is very specialized utility class, why do we need to promote it to core?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per #5910. Consider a plugin that has it's own custom Analyzer using AnalysisPlugin. Downstream plugins implement a TokenFilterFactory which needs Lucene's TokenStream. We need to take a dependency on lucene for this in whatever library houses those contracts (currently decided to be the opensearch-core library).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Separately from that, I could remove BytesRefUtils altogether as that logic isn't actually used anywhere. I chose to keep it (for now) as a convenience function for external plugins.

Copy link
Collaborator

@reta reta Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We need to take a dependency on lucene for this in whatever library houses those contracts (currently decided to be the opensearch-core library).

This is totally fine, and I believe by importing AnalysisPlugin the Lucene will be brought in, but the core does not need it and should not bring it I think. Take libs for example - they hardly ever need to know about Lucene but very likely will depend on core / common.

Separately from that, I could remove BytesRefUtils altogether as that logic isn't actually used anywhere.

👍 ty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...but the core does not need it and should not bring it I think

I think we're not on the same page. #5910 plans to move general contracts like AnalysisPlugin (and all of the others) out of :server and into opensearch-core library so plugins only need take a dependency on the API not the entire :server module. To do that requires classes like *Plugin, be refactored out of :server and into libs:opensearch-core. If you refactor AnalysisPlugin to :opensearch-core you need :opensearch-core to take a dependency on lucene. So, the core library will need this lucene dependency, otherwise these contracts have to stay in :server

Copy link
Collaborator

@reta reta Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What's the different route?

First thing, remove dependency to opensearch-core where opensearch-common would be sufficient (possibly moving few classes between modules). With that - we could be 1000% sure that no new transitive dependencies would surprise us along the away.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the meantime I updated this PR to remove opensearch-core dependency from ssl-config and nio libraries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nknize libs/opensearch-cli ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1 done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was hoping all of this would be done incrementally after this PR was merged.

There is an argument for moving the xcontent classes out of core into the common library now and letting core take the dependency on Lucene and other third-party libraries. Which would mean all of the plugins would revert the PRs they just merged. This is why we need to decouple downstream repos to enable us to move on core changes.


testImplementation "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
testImplementation "junit:junit:${versions.junit}"
testImplementation "org.hamcrest:hamcrest:${versions.hamcrest}"
Expand All @@ -96,4 +99,5 @@ tasks.named('forbiddenApisMain').configure {

tasks.named("dependencyLicenses").configure {
mapping from: /jackson-.*/, to: 'jackson'
mapping from: /lucene-.*/, to: 'lucene'
}
Loading