-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Cross Cluster Replication support #3533
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
Conversation
russcam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments
| namespace Nest | ||
| { | ||
| /// <summary> | ||
| /// This is a custom dictionary that helps in the creation of remote cluster configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just
Configuration used to configure a remote cluster
?
| namespace Nest | ||
| { | ||
| /// <summary> | ||
| /// This API creates a new named collection of auto-follow patterns against the remote cluster specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the "This API" from the beginning
| public partial interface IElasticClient | ||
| { | ||
| /// <summary> | ||
| /// This API creates a new named collection of auto-follow patterns against the remote cluster specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "This API" from beginning
| { | ||
| public partial interface IElasticClient | ||
| { | ||
| /// <summary> This API gets configured auto-follow patterns. This API will return the specified auto-follow pattern collection. </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "This API" from beginning. Change "This API will return..." to "Returns..."
|
|
||
| namespace Nest | ||
| { | ||
| /// <summary> This API gets configured auto-follow patterns. This API will return the specified auto-follow pattern collection. </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "This API" from beginning. Change "This API will return..." to "Returns..."
| namespace Nest | ||
| { | ||
| /// <summary> | ||
| /// This API stops the following task associated with a follower index and removes index metadata and settings associated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "This API" from the beginning
| namespace Nest | ||
| { | ||
| /// <summary> | ||
| /// This API gets cross-cluster replication stats. This API will return all stats related to cross-cluster replication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "This API" from the beginning
| public partial interface IElasticClient | ||
| { | ||
| /// <summary> | ||
| /// This API gets cross-cluster replication stats. This API will return all stats related to cross-cluster replication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "This API" from the beginning
| namespace Tests.XPack.CrossClusterReplication | ||
| { | ||
| [SkipVersion("<6.5.0", "")] | ||
| public class CrossClusterReplicationFollowTests : CoordinatedIntegrationTestBase<WritableCluster> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot going on in this integration test! I think it may be useful to add a <summary> that provides a brief overview of what is being tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, each step is dependent on the "setup" of the previous one, and is asserted in its own integration test method in the class.
If my understanding is correct, maybe we could do something similar here as to what exists for ML job API integration tests - a base class with setup steps that can be called in IntegrationSetup method for each deriving integration test, and a derived test class for each separate API?
| namespace Tests.XPack.CrossClusterReplication | ||
| { | ||
| [SkipVersion("<6.5.0", "")] | ||
| public class CrossClusterReplicationAutoFollowTests : CoordinatedIntegrationTestBase<WritableCluster> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to other comment:
As I understand it, each step is dependent on the "setup" of the previous one, and is asserted in its own integration test method in the class.
If my understanding is correct, maybe we could do something similar here as to what exists for ML job API integration tests - a base class with setup steps that can be called in IntegrationSetup method for each deriving integration test, and a derived test class for each separate API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye, The ML approach is valid since most operate on a job so their IntegrationSetup is relatively straightforward and inheritable.
The setup in ML run constantly though for each ApiTest test class.
CCR is more a chain of API's to call, where the same approach would leave a lot of unfinished setup chains e.g CoodinatedIntegrationTest allows us to express this dependency chain:
A1 => A2 => A3 => A4 = > A5 => A6
Which will be only executed once for each permutation of [async, sync] and [fluent, ois]
if we take the object graph approach to test each AN in isolation
We end up with:
A1
A1 => A2
A1 => A2 => A3
A1 => A2 => A3 => A4
A1 => A2 => A3 => A4 = > A5
A1 => A2 => A3 => A4 = > A5 => A6
Foreach permutation of [async, sync] and [fluent, ois].
|
@russcam thank you for the review, I've updated the xmldocs to be less
|
* Initial code gen for CCR * Created all the infra for the CCR api's (request/response/client methods) * Progress commit implementing the API and coordinated integration test for it * Add global ccr stats endpoint and tidy up implemented ccr api's thus far * Auto Follow CCR api's + tests * default keyword * CCR url and api tests * This API comments
* Initial code gen for CCR * Created all the infra for the CCR api's (request/response/client methods) * Progress commit implementing the API and coordinated integration test for it * Add global ccr stats endpoint and tidy up implemented ccr api's thus far * Auto Follow CCR api's + tests * default keyword * CCR url and api tests * This API comments
* Initial code gen for CCR * Created all the infra for the CCR api's (request/response/client methods) * Progress commit implementing the API and coordinated integration test for it * Add global ccr stats endpoint and tidy up implemented ccr api's thus far * Auto Follow CCR api's + tests * default keyword * CCR url and api tests * This API comments (cherry picked from commit 13c6a80)
* Initial code gen for CCR * Created all the infra for the CCR api's (request/response/client methods) * Progress commit implementing the API and coordinated integration test for it * Add global ccr stats endpoint and tidy up implemented ccr api's thus far * Auto Follow CCR api's + tests * default keyword * CCR url and api tests * This API comments (cherry picked from commit 13c6a80)
This adds support for all the CCR related API's
This also introduces
CoordinatedIntegrationTestBase<>as a means to execute integration tests in a forced sequence for all api overloads.Further more it introduces
RemoteClusterConfigurationto simplify specifying remote clusters as part of the cluster settings API for use with Cross Cluster replication and searches.You can now also enable soft deletes on index settings in a strongly typed fashion.