[Refactor] StreamInput, StreamOutput, Writeable to core library by nknize · Pull Request #7783 · opensearch-project/OpenSearch

nknize · 2023-05-26T20:46:56Z

This is a big commit as it refactors the StreamInput and StreamOutput core serialization and supporting classes from the server module to the core library such that it can be reused and overridden in cloud native and serverless applications. The majority of the changes in this commit are the import statements. Noteable refactors include:

StreamInput
StreamOutput
Writeable
NamedWriteable
NamedWriteableRegistry
Index
ShardId
BytesReference
ByteArray
BigArray
SecureString
Text
ParsingException
RestStatus

NOTE: The below dependency PRs reduce the surface area of this commit. However, it will still be a large commit as reducing to incremental commits adds a substantial intermediate "generics" framework that only serves to complicate the implementation. Thus we instead opt to solve the refactor in one swooping change.

relates #5910
depends on #7646
depends on #7780
depends on #8035

github-actions · 2023-05-26T20:54:45Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/16393/
CommitID: 2182f3d
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-26T21:17:00Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/16395/
CommitID: b30610c
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-29T16:18:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: null
CommitID: 0336cf0
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-29T16:41:57Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: null
CommitID: 91be372
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-29T17:27:37Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: null
CommitID: 91be372
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-30T03:31:05Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/16504/
CommitID: 91be372
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-06-07T21:03:13Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/17073/
CommitID: d315080
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-06-07T21:07:05Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/17075/
CommitID: 4931950
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-06-07T21:52:39Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/17078/
CommitID: d56496c
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?

OpenSearchException uses an enumerator to create a handler for each serializable Exception implementation. The enumerator is to make it easy to store in an immutable map. The problem with this Hack is that it makes it difficult to unwind the tight coupling of StreamInput with the OpenSearchException class. This commit switches from using an enumerator to a registry in the opensearch-core library in order to support serverless and cloud native implementations outside of the server module. This commit also refactors base primitive serialization readers and writers from StreamInput and StreamOutput to BaseStreamInput and BaseStreamOutput, respectively. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

This is a big commit as it refactors the StreamInput and StreamOutput core serialization and supporting classes from the server module to the core library such that it can be reused and overridden in cloud native and serverless applications. The majority of the changes in this commit are the import statements. Noteable refactors include: * StreamInput * StreamOutput * Writeable * NamedWriteable * NamedWriteableRegistry * Index * ShardId * BytesReference * ByteArray * BigArray * SecureString * Text * ParsingException * RestStatus Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

github-actions · 2023-06-12T16:56:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/17327/
CommitID: 18477fd
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?

reta · 2023-06-12T17:03:29Z

client/rest-high-level/src/main/java/org/opensearch/client/indices/CloseIndexResponse.java

            PARSER.declareObject(optionalConstructorArg(), (p, c) -> {
-                XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, p.currentToken(), p);
-                XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, p.nextToken(), p);
+                org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken(


Seems like the full import is there already:

Suggested change

org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken(

XContentParserUtils.ensureExpectedToken(

reta · 2023-06-12T17:03:51Z

client/rest-high-level/src/main/java/org/opensearch/client/indices/CloseIndexResponse.java

+                    p.currentToken(),
+                    p
+                );
+                org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, p.nextToken(), p);


Suggested change

org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, p.nextToken(), p);

XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, p.nextToken(), p);

reta · 2023-06-12T17:10:15Z

libs/core/src/main/java/org/opensearch/BaseOpenSearchException.java

+    static {
+        registerExceptionHandle(
+            new BaseOpenSearchExceptionHandle(
+                org.opensearch.core.index.snapshots.IndexShardSnapshotFailedException.class,


Why snapshots are moving core? I am not sure these functionality belong to it

reta · 2023-06-12T17:20:33Z

libs/core/src/main/java/org/opensearch/BaseOpenSearchException.java

+     *
+     * @return an array of all registered handle IDs
+     */
+    static int[] ids() {


Why do we need to duplicate that vs calling OpenSearchExceptionHandleRegistry.ids()?

reta · 2023-06-12T17:28:50Z

libs/core/src/main/java/org/opensearch/core/common/io/StreamsUtil.java

+ * @opensearch.internal
+ */
+public class StreamsUtil {
+    public static int readFully(InputStream reader, byte[] dest, int offset, int len) throws IOException {


InputStream::readNBytes does exactly that, no need for additional utils:

reader.readNBytes(dest, offset, len);

reta · 2023-06-12T17:36:00Z

libs/core/src/main/java/org/opensearch/core/common/io/stream/StreamInput.java

        return new BytesRef(bytes, 0, length);
    }

+    public void readFully(byte[] b) throws IOException {


This method won't consume full stream, it will consume up to b length, I think readBytes (overloaded version) would be a better name:

public void readBytes(byte[] b)

nknize · 2023-06-19T22:15:51Z

After merging #8035 it was easier to open a new PR #8157 instead of rebasing and dealing with merge conflicts. I will plan to close this PR once #8157 lands.

rishabhmaurya · 2023-06-22T00:00:58Z

libs/core/src/main/java/org/opensearch/core/common/bytes/AbstractBytesReference.java


        @Override
-        protected void ensureCanReadBytes(int bytesToRead) throws EOFException {
+        public void ensureCanReadBytes(int bytesToRead) throws EOFException {


moving classes around may require changing access modifiers but comes with a security risk. This one seems harmless however we can move most the of consumers of this classes to lib:core from server. I also see its unit tests are still in server.
If we are not intending to move unit tests and other consumers of this class here, then we should create a list of TODOs, to change them back to their original access modifiers once consumers are moved to lib

Yeah @rishabhmaurya, I actually like the idea of refactoring these classes as well. The reason I held off is because one of those inheritors, BufferedChecksumStreamInput, is inside the o.o.index.translog namespace and we may want to consider having this in a dedicated :libs:opensearch-translog library instead of core so we don't set a precedent of having translog logic in the core library. Especially since we're talking about scenarios where we don't have a translog.

Oh by the way.. this PR is going to get closed in favor of #8157 so have a look at that one instead if you wouldn't mind.

nknize · 2023-07-11T20:52:41Z

superseded by #8035

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

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

nknize force-pushed the streamio branch from 0336cf0 to 91be372 Compare May 29, 2023 16:41

nknize added 13 commits June 12, 2023 11:18

fix missingJavaDoc failures

76348c7

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

fix missingjavadoc again

6d61b02

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

rebase from in flight changes

f07176e

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

cleanup and merge

37ea33c

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

fix failures

0fb207f

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

rebase and cleanup

4521440

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

spotless

5fe99e7

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

more test fixes

b5486ef

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

integration test fixes

87c003c

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

integ fixes

86583a0

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

fix merge conflicts

18477fd

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

nknize force-pushed the streamio branch from d56496c to 18477fd Compare June 12, 2023 16:29

reta reviewed Jun 12, 2023

View reviewed changes

This was referenced Jun 13, 2023

[Refactor] StreamIO and OpenSearchException foundation to core library #8035

Merged

[Refactor] StreamIO from common to core.common namespace in core lib #8157

Merged

rishabhmaurya reviewed Jun 22, 2023

View reviewed changes

nknize closed this Jul 11, 2023

	org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken(
	XContentParserUtils.ensureExpectedToken(

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented May 26, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 26, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 29, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 29, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 29, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented May 30, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jun 7, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jun 7, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jun 7, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jun 12, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

reta Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

reta Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

reta Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reta Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

nknize commented Jun 19, 2023

Uh oh!

rishabhmaurya Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nknize Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

nknize commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nknize commented May 26, 2023 •

edited

Loading

reta Jun 12, 2023 •

edited

Loading

reta Jun 12, 2023 •

edited

Loading

rishabhmaurya Jun 22, 2023 •

edited

Loading

nknize Jun 22, 2023 •

edited

Loading