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

Initial commit for index routing table manifest #13255

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Bukhtawar
Copy link
Collaborator

Description

Adds index routing info to the cluster metadata manifest

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Copy link
Contributor

❌ Gradle check result for 4afb08b: 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: Bukhtawar Khan <bukhtawa@amazon.com>
Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Copy link
Contributor

❌ Gradle check result for b38752c: 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?

Copy link
Contributor

❌ Gradle check result for f6eda99: 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: Bukhtawar Khan <bukhtawa@amazon.com>
Copy link
Contributor

❌ Gradle check result for c6ed63f: 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?

@@ -319,6 +391,8 @@ public void writeTo(StreamOutput out) throws IOException {
if (out.getVersion().onOrAfter(Version.V_2_12_0)) {
out.writeInt(codecVersion);
out.writeString(globalMetadataFileName);
} else if (out.getVersion().onOrAfter(Version.V_2_14_0)) {

Choose a reason for hiding this comment

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

We would need to change to if instead of else if so both blocks are executed for versions after 2.14

private void initialFill() throws IOException {
BytesReference bytesReference = indexRoutingTableHeader.write();
buf = bytesReference.toBytesRef().bytes;
count = bytesReference.length();
Copy link
Member

Choose a reason for hiding this comment

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

With this the initialization of buf in ctor is not used. Also, the buffer size will not be size from this point onwards but would be count

leftOverBuf = null;
} else {
System.arraycopy(bytesRef.toBytesRef().bytes, 0, buf, count, buf.length - count);
count += buf.length - count;
Copy link
Member

Choose a reason for hiding this comment

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

This essentially becomes count = buf.length, right?

} else {
System.arraycopy(bytesRef.toBytesRef().bytes, 0, buf, count, buf.length - count);
count += buf.length - count;
leftOverBuf = new byte[bytesRef.length() - count];
Copy link
Member

Choose a reason for hiding this comment

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

This line should be before changing the value of count. Also, it should be:

leftOverBuf = new byte[bytesRef.length() - (buf.length - count)];

System.arraycopy(bytesRef.toBytesRef().bytes, 0, buf, count, buf.length - count);
count += buf.length - count;
leftOverBuf = new byte[bytesRef.length() - count];
System.arraycopy(bytesRef.toBytesRef().bytes, buf.length - count + 1, leftOverBuf, 0, bytesRef.length() - count);
Copy link
Member

Choose a reason for hiding this comment

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

Need to change values of 2nd and 5th parameters.

import java.io.InputStream;
import java.util.Iterator;

public class IndexRoutingTableInputStream extends InputStream {
Copy link
Member

Choose a reason for hiding this comment

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

This is a useful extension. Just thinking if this can be made generic where, in this case, T would be IndexRoutingTable?

@@ -237,6 +275,25 @@ public ClusterMetadataManifest(
String previousClusterUUID,
boolean clusterUUIDCommitted
) {
this(clusterTerm, version, clusterUUID, stateUUID, opensearchVersion, nodeId, committed, codecVersion,
globalMetadataFileName, indices, previousClusterUUID, clusterUUIDCommitted, null);

Choose a reason for hiding this comment

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

Passing indicesRouting as null will throw NPE in the next step when converting to Collections.unmodifiableList

Comment on lines +369 to +374
builder.startArray(INDICES_ROUTING_FIELD.getPreferredName());
{
for (UploadedIndexMetadata uploadedIndexMetadata : indicesRouting) {
uploadedIndexMetadata.toXContent(builder, params);
}
}

Choose a reason for hiding this comment

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

builder.endArray needs to be called at the end after adding all uploadedIndexMetadata

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled Issues that have stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants