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

[Segment Replication] Implementing cat/segment_replication API #5718

Merged
merged 37 commits into from
Feb 8, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
2eed8a8
Initial Draft for adding segment_replication API
Rishikesh1159 Jan 5, 2023
88cf749
Adding bytes transfered in each segrep events and additional metrics.
Rishikesh1159 Jan 9, 2023
888c9b3
Merge branch 'main' into segment_replication_API
Rishikesh1159 Jan 24, 2023
ba048a4
Fix broken tests.
Rishikesh1159 Jan 24, 2023
69bc392
Merge branch 'segment_replication_API' of https://github.com/Rishikes…
Rishikesh1159 Jan 24, 2023
b062420
Fix compile errors
Rishikesh1159 Jan 24, 2023
1728ae7
Adding Tests and gating logic behind feature flag.
Rishikesh1159 Jan 25, 2023
b10519e
Add java docs and enable query parameter detailed.
Rishikesh1159 Jan 26, 2023
74a41b0
Add temporary documentation URL
Rishikesh1159 Jan 26, 2023
7752a00
Fixing failing tests.
Rishikesh1159 Jan 27, 2023
594814e
Merge branch 'main' into segment_replication_API
Rishikesh1159 Jan 27, 2023
9e62cd4
Merge branch 'segment_replication_API' of https://github.com/Rishikes…
Rishikesh1159 Jan 27, 2023
b5432bd
Spotless Apply.
Rishikesh1159 Jan 27, 2023
2b92c81
Fix media type copile check.
Rishikesh1159 Jan 27, 2023
5c9f8a7
Revert previous changes and fix failing tests.
Rishikesh1159 Jan 29, 2023
e022a7e
Apply spotless check.
Rishikesh1159 Jan 29, 2023
8b26e2e
Refactoring call to segmentreplicationstate.
Rishikesh1159 Jan 29, 2023
dd4f5b1
spotless check
Rishikesh1159 Jan 29, 2023
0e52667
Merge branch 'opensearch-project:main' into segment_replication_API
Rishikesh1159 Jan 29, 2023
c85e38a
Changing invokation of segment replication shard and filtering API re…
Rishikesh1159 Jan 31, 2023
ad1fd5b
Merge branch 'segment_replication_API' of https://github.com/Rishikes…
Rishikesh1159 Jan 31, 2023
2ef7a9f
disable feature flag by default.
Rishikesh1159 Jan 31, 2023
9a4f09d
Apply spotless
Rishikesh1159 Jan 31, 2023
1080536
Address comments on PR.
Rishikesh1159 Feb 3, 2023
86cf0ae
Merge branch 'main' into segment_replication_API
Rishikesh1159 Feb 3, 2023
8ae10c8
Merge branch 'segment_replication_API' of https://github.com/Rishikes…
Rishikesh1159 Feb 3, 2023
9cf580e
Merge branch 'opensearch-project:main' into segment_replication_API
Rishikesh1159 Feb 3, 2023
b8f7b71
Fix gradle check failures
Rishikesh1159 Feb 3, 2023
08bc3d6
Merge branch 'segment_replication_API' of https://github.com/Rishikes…
Rishikesh1159 Feb 3, 2023
c1ef1d4
fix failing testSegment_ReplicationActionAction()
Rishikesh1159 Feb 3, 2023
884e6d1
Exclude empty segment replication events in API response.
Rishikesh1159 Feb 6, 2023
537964f
Apply spotless.
Rishikesh1159 Feb 6, 2023
f120a92
Address PR comments and add Integ Tests.
Rishikesh1159 Feb 7, 2023
35460e9
Merge branch 'opensearch-project:main' into segment_replication_API
Rishikesh1159 Feb 7, 2023
fc24661
Fix failing testSegmentReplicationApiResponse().
Rishikesh1159 Feb 7, 2023
f64f14c
Merge branch 'segment_replication_API' of https://github.com/Rishikes…
Rishikesh1159 Feb 7, 2023
7d07dc6
Refactoring code.
Rishikesh1159 Feb 8, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
{
"cat.segment_replication":{
"documentation":{
"url":"https://www.elastic.co/guide/en/elasticsearch/reference/master/cat-segment_replication.html",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be including any further links to elasticsearch documentation anymore. This should be redirected to opensearch documentation and if not present, can open an issue in opensearch-project/documentation-website to create the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @Poojita-Raj for catching this, yes you are right. I have temporarily added url of opensearch documentation for cat recovery API, I will update this once I open an issue with opensearch-project/documentation-website

"description":"Returns information about index Segment Replication events, both on-going and completed."
Copy link
Member

Choose a reason for hiding this comment

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

how many completed?

},
"stability":"stable",
Copy link
Member

Choose a reason for hiding this comment

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

@Rishikesh1159 Should probably make this experimental, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @andrross for catching this. Yes I missed this, it should be experimental. Let me make a PR to change this.

"url":{
"paths":[
{
"path":"/_cat/segment_replication",
"methods":[
"GET"
]
},
{
"path":"/_cat/segment_replication/{index}",
"methods":[
"GET"
],
"parts":{
"index":{
"type":"list",
"description":"Comma-separated list or wildcard expression of index names to limit the returned information"
}
}
}
]
},
"params":{
"format":{
"type":"string",
"description":"a short version of the Accept header, e.g. json, yaml"
},
"active_only":{
Copy link
Member

Choose a reason for hiding this comment

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

nice

"type":"boolean",
"description":"If `true`, the response only includes ongoing segment replications",
"default":false
},
"bytes":{
"type":"enum",
"description":"The unit in which to display byte values",
"options":[
"b",
"k",
"kb",
"m",
"mb",
"g",
"gb",
"t",
"tb",
"p",
"pb"
]
},
"detailed":{
"type":"boolean",
"description":"If `true`, the response includes detailed information about segment replications",
"default":false
},
"h":{
"type":"list",
"description":"Comma-separated list of column names to display"
},
"help":{
"type":"boolean",
"description":"Return help information",
"default":false
},
"index":{
"type":"list",
"description":"Comma-separated list or wildcard expression of index names to limit the returned information"
},
"s":{
"type":"list",
"description":"Comma-separated list of column names or column aliases to sort by"
},
"time":{
"type":"enum",
"description":"The unit in which to display time values",
"options":[
"d",
"h",
"m",
"s",
"ms",
"micros",
"nanos"
]
},
"v":{
"type":"boolean",
"description":"Verbose mode. Display column headers",
"default":false
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
import org.opensearch.action.admin.indices.recovery.RecoveryRequest;
import org.opensearch.action.admin.indices.refresh.RefreshRequest;
import org.opensearch.action.admin.indices.refresh.TransportShardRefreshAction;
import org.opensearch.action.admin.indices.segment_replication.SegmentReplicationAction;
import org.opensearch.action.admin.indices.segment_replication.SegmentReplicationRequest;
import org.opensearch.action.admin.indices.segments.IndicesSegmentsAction;
import org.opensearch.action.admin.indices.segments.IndicesSegmentsRequest;
import org.opensearch.action.admin.indices.settings.get.GetSettingsAction;
Expand Down Expand Up @@ -468,6 +470,17 @@ public void testRecovery() {
assertSameIndices(recoveryRequest, recoveryAction);
}

public void testSegment_Replication() {
String segmentReplicationAction = SegmentReplicationAction.NAME + "[n]";
interceptTransportActions(segmentReplicationAction);

SegmentReplicationRequest segmentReplicationRequest = new SegmentReplicationRequest(randomIndicesOrAliases());
internalCluster().coordOnlyNodeClient().admin().indices().segment_replication(segmentReplicationRequest).actionGet();

clearInterceptedActions();
assertSameIndices(segmentReplicationRequest, segmentReplicationAction);
}

public void testSegments() {
String segmentsAction = IndicesSegmentsAction.NAME + "[n]";
interceptTransportActions(segmentsAction);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@
import org.opensearch.action.admin.indices.recovery.RecoveryResponse;
import org.opensearch.action.admin.indices.stats.IndicesStatsAction;
import org.opensearch.action.admin.indices.stats.IndicesStatsResponse;
import org.opensearch.cluster.metadata.IndexMetadata;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.unit.TimeValue;
import org.opensearch.indices.replication.common.ReplicationType;
import org.opensearch.plugins.Plugin;
import org.opensearch.test.OpenSearchIntegTestCase;
import org.opensearch.test.transport.MockTransportService;
Expand Down Expand Up @@ -147,6 +149,46 @@ public void testRecoveriesWithTimeout() {
assertThat(recoveryResponse.getShardFailures()[0].reason(), containsString("ReceiveTimeoutTransportException"));
}

public void testSegment_ReplicationWithTimeout() {
internalCluster().startClusterManagerOnlyNode();
String dataNode = internalCluster().startDataOnlyNode();
String anotherDataNode = internalCluster().startDataOnlyNode();

int numShards = 4;
assertAcked(
prepareCreate(
"test-index",
0,
Settings.builder()
.put("number_of_shards", numShards)
.put("number_of_replicas", 1)
.put(IndexMetadata.SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT)
)
);
ensureGreen();
final long numDocs = scaledRandomIntBetween(50, 100);
for (int i = 0; i < numDocs; i++) {
index("test-index", "doc", Integer.toString(i));
}
refresh("test-index");
ensureSearchable("test-index");

// Happy case
RecoveryResponse recoveryResponse = dataNodeClient().admin().indices().prepareRecoveries().get();
assertThat(recoveryResponse.getTotalShards(), equalTo(numShards * 2));
assertThat(recoveryResponse.getSuccessfulShards(), equalTo(numShards * 2));

// simulate timeout on bad node.
simulateTimeoutAtTransport(dataNode, anotherDataNode, RecoveryAction.NAME);

// verify response with bad node.
recoveryResponse = dataNodeClient().admin().indices().prepareRecoveries().get();
assertThat(recoveryResponse.getTotalShards(), equalTo(numShards * 2));
assertThat(recoveryResponse.getSuccessfulShards(), equalTo(numShards));
assertThat(recoveryResponse.getFailedShards(), equalTo(numShards));
assertThat(recoveryResponse.getShardFailures()[0].reason(), containsString("ReceiveTimeoutTransportException"));
}

public void testStatsWithTimeout() {
internalCluster().startClusterManagerOnlyNode();
String dataNode = internalCluster().startDataOnlyNode();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@
import org.opensearch.action.admin.indices.resolve.ResolveIndexAction;
import org.opensearch.action.admin.indices.rollover.RolloverAction;
import org.opensearch.action.admin.indices.rollover.TransportRolloverAction;
import org.opensearch.action.admin.indices.segment_replication.SegmentReplicationAction;
import org.opensearch.action.admin.indices.segment_replication.TransportSegmentReplicationAction;
import org.opensearch.action.admin.indices.segments.IndicesSegmentsAction;
import org.opensearch.action.admin.indices.segments.PitSegmentsAction;
import org.opensearch.action.admin.indices.segments.TransportIndicesSegmentsAction;
Expand Down Expand Up @@ -397,6 +399,7 @@
import org.opensearch.rest.action.cat.RestAllocationAction;
import org.opensearch.rest.action.cat.RestCatAction;
import org.opensearch.rest.action.cat.RestCatRecoveryAction;
import org.opensearch.rest.action.cat.RestCatSegmentReplicationAction;
import org.opensearch.rest.action.cat.RestFielddataAction;
import org.opensearch.rest.action.cat.RestHealthAction;
import org.opensearch.rest.action.cat.RestIndicesAction;
Expand Down Expand Up @@ -649,6 +652,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void reg
actions.register(ExplainAction.INSTANCE, TransportExplainAction.class);
actions.register(ClearScrollAction.INSTANCE, TransportClearScrollAction.class);
actions.register(RecoveryAction.INSTANCE, TransportRecoveryAction.class);
actions.register(SegmentReplicationAction.INSTANCE, TransportSegmentReplicationAction.class);
actions.register(NodesReloadSecureSettingsAction.INSTANCE, TransportNodesReloadSecureSettingsAction.class);
actions.register(AutoCreateAction.INSTANCE, AutoCreateAction.TransportAction.class);

Expand Down Expand Up @@ -870,6 +874,7 @@ public void initRestHandlers(Supplier<DiscoveryNodes> nodesInCluster) {

// CAT API
registerHandler.accept(new RestAllocationAction());
registerHandler.accept(new RestCatSegmentReplicationAction());
registerHandler.accept(new RestShardsAction());
registerHandler.accept(new RestClusterManagerAction());
registerHandler.accept(new RestNodesAction());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.action.admin.indices.segment_replication;

import org.opensearch.action.ActionType;

public class SegmentReplicationAction extends ActionType<SegmentReplicationResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: might need a javadoc/comment here

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in latest commit

Copy link
Member

Choose a reason for hiding this comment

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

nit - Naming doesn't indicate this is about stats. What about SegmentReplicationStatsAction/Response ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes SegmentReplicationStatsAction makes sense. I will update that in next commit

public static final SegmentReplicationAction INSTANCE = new SegmentReplicationAction();
public static final String NAME = "indices:monitor/segment_replication";

private SegmentReplicationAction() {
super(NAME, SegmentReplicationResponse::new);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.action.admin.indices.segment_replication;

import org.opensearch.action.support.IndicesOptions;
import org.opensearch.action.support.broadcast.BroadcastRequest;
import org.opensearch.common.Strings;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;

import java.io.IOException;

public class SegmentReplicationRequest extends BroadcastRequest<SegmentReplicationRequest> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: javadocs/comments

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in latest commit

private boolean detailed = false; // Provides extra details in the response
private boolean activeOnly = false; // Only reports on active recoveries

/**
* Constructs a request for segment replication information for all shards
*/
public SegmentReplicationRequest() {
this(Strings.EMPTY_ARRAY);
}

public SegmentReplicationRequest(StreamInput in) throws IOException {
super(in);
detailed = in.readBoolean();
activeOnly = in.readBoolean();
}

/**
* Constructs a request for segment replication information for all shards for the given indices
*
* @param indices Comma-separated list of indices about which to gather segment replication information
*/
public SegmentReplicationRequest(String... indices) {
super(indices, IndicesOptions.STRICT_EXPAND_OPEN_CLOSED);
}

/**
* True if detailed flag is set, false otherwise. This value if false by default.
*
* @return True if detailed flag is set, false otherwise
*/
public boolean detailed() {
return detailed;
}

/**
* Set value of the detailed flag. Detailed requests will contain extra
* information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be more helpful to mention exactly what this detailed information will be. Could be included in the help message for detailed flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I have added that in latest commit, detailed information will be timing metric of each stage of segment replication event. Sure, I will try to add this in help message in next commit

*
* @param detailed Whether or not to set the detailed flag
*/
public void detailed(boolean detailed) {
this.detailed = detailed;
}

/**
* True if activeOnly flag is set, false otherwise. This value is false by default.
*
* @return True if activeOnly flag is set, false otherwise
*/
public boolean activeOnly() {
return activeOnly;
}

/**
* Set value of the activeOnly flag. If true, this request will only response with
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: *respond with

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in latest commit

* on-going recovery information.
*
* @param activeOnly Whether or not to set the activeOnly flag.
*/
public void activeOnly(boolean activeOnly) {
this.activeOnly = activeOnly;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeBoolean(detailed);
out.writeBoolean(activeOnly);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.action.admin.indices.segment_replication;

import org.opensearch.action.support.broadcast.BroadcastOperationRequestBuilder;
import org.opensearch.client.OpenSearchClient;

public class SegmentReplicationRequestBuilder extends BroadcastOperationRequestBuilder<
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some javadocs/comments might be useful here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in latest commit

SegmentReplicationRequest,
SegmentReplicationResponse,
SegmentReplicationRequestBuilder> {

public SegmentReplicationRequestBuilder(OpenSearchClient client, SegmentReplicationAction action) {
super(client, action, new SegmentReplicationRequest());
}

public SegmentReplicationRequestBuilder setDetailed(boolean detailed) {
request.detailed(detailed);
return this;
}

public SegmentReplicationRequestBuilder setActiveOnly(boolean activeOnly) {
request.activeOnly(activeOnly);
return this;
}

}
Loading