[Remote Routing Table] Initial commit for index routing table manifest #13577
Conversation
Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Signed-off-by: Himshikha Gupta <himshikh@amazon.com>
|
❌ Gradle check result for 441f520: FAILURE 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? |
| this.bytesStreamOutput = new BytesStreamOutput(); | ||
| this.out = new BufferedChecksumStreamOutput(bytesStreamOutput); |
There was a problem hiding this comment.
Why do we require them to be properties of the class. All that we should care about is the byte array that acts as a buffer to get reused across operations
There was a problem hiding this comment.
BytesStreamOutput already uses a ByteArray reference which kind of defeats the the purpose of having buf
There was a problem hiding this comment.
We need the checksum to be added for complete file, for this we need to ensure we reuse the same BufferedChecksumStreamOutput throughout the write operations.
We are resetting the BytesStreamOutput once we copy the content in buf, this ensures the BytesStreamOutput doesn't accumulate the whole data and we manage the buffer size.
There was a problem hiding this comment.
Exactly so BufferedChecksumStreamOutput should be used only within the context of a method or specific operation as a temp storage rather than exposing it as a class property
There was a problem hiding this comment.
The issue with keeping BufferedChecksumStreamOutput as a temp storage is we will then need to retain and update checksum independently.
The usage of this class is something like:
inputStream = new IndexRoutingTableInputStream() -> which calls initialFill where we put some content in buffer.
Post this inputStream.read() will be called in a loop till we have content in buffer.
In both these methods we need to retain the checksum and add to it with the next content read. BufferedChecksumStreamOutput does not currently expose any methods to add checksum to an initial value.
The way I see we have 2 options here:
- Keep using
BufferedChecksumStreamOutputas class property which takes care of updating checksum across reads. - Retain checksum from one read and update
BufferedChecksumStreamOutputto be able to use an initial value in the next read.
Please suggest if there is a better way of handling checksum or if I am missing something here.
| @@ -30,7 +30,7 @@ | |||
| * GitHub history for details. | |||
| */ | |||
|
|
|||
| package org.opensearch.index.translog; | |||
There was a problem hiding this comment.
The split package issue (Java package org.opensearch.comm being used by both :server and :libs/common) is a problem and a blocker for adopting JPMS. I think we should avoid exacerbating that problem by adding more things in org.opensearch.common in the :server module. I suggest either not moving it, moving this elsewhere :server, or move it to org.opensearch.core.common in :libs/core.
There was a problem hiding this comment.
Shouldn't all commons go into libs/common?
Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
|
❌ Gradle check result for ee70dca: FAILURE 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? |
|
❌ Gradle check result for 9efd85b: FAILURE 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? |
Signed-off-by: Himshikha Gupta <himshikh@amazon.com>
himshikha
force-pushed
the
remote_routing_input_stream
branch
from
9efd85b
to
7aeecc8
Compare
|
❌ Gradle check result for 7aeecc8: FAILURE 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? |
Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
|
❌ Gradle check result for 7b2bc79: FAILURE 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? |
Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
|
❌ Gradle check result for 88d266f: FAILURE 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? |
Signed-off-by: Himshikha Gupta <himshikh@amazon.com>
himshikha
force-pushed
the
remote_routing_input_stream
branch
from
1ef13ae
to
4c97869
Compare
|
❌ Gradle check result for 1ef13ae: FAILURE 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? |
|
❌ Gradle check result for 4c97869: FAILURE 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? |
Description
This PR is based off on the PR by @Bukhtawar for index routing stream setup #13255
This fixes some of the bugs in input stream creation in the original PR. Additionally, this moves checksum creation to end of file and we will create single checksum for complete file rather than having individual checksum.
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
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.