-
Notifications
You must be signed in to change notification settings - Fork 26
Add support for PUT _ccr/follow #322
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
jherns
commented
Feb 10, 2025
- Add support for following an index
- Update test framework to support CCR
- will skip CCR tests if the replica isn't reachable
- Update readme
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.
PR Overview
This pull request adds support for following an index using the PUT _ccr/follow API and updates test infrastructure and documentation for CCR.
- Adds a new CCR client module with a follow method in the client library
- Introduces tests for following a leader index and sets up a replica client in the test helper
- Updates the README with CCR testing instructions
Changes
| File | Description |
|---|---|
| test/client/ccr_test.rb | Adds tests to verify following a leader index using CCR |
| lib/elastomer_client/client/ccr.rb | Introduces the new CCR module and the follow method implementation |
| test/test_helper.rb | Configures the replica client for CCR tests |
| README.md | Updates usage instructions for CCR tests |
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
lib/elastomer_client/client/ccr.rb
Outdated
| # | ||
| # See https://www.elastic.co/guide/en/elasticsearch/reference/current/ccr-put-follow.html | ||
| def follow(follower_index, body, params = {}) | ||
| response = client.put "/#{follower_index}/_ccr/follow", params.merge(params, body:, action: "follow", rest_api: "ccr") |
Copilot
AI
Feb 10, 2025
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.
The use of params.merge(params, ...) appears redundant and may lead to unexpected behavior. Consider simplifying it to params.merge(body: body, action: "follow", rest_api: "ccr") to avoid merging the same hash twice.
| response = client.put "/#{follower_index}/_ccr/follow", params.merge(params, body:, action: "follow", rest_api: "ccr") | |
| response = client.put "/#{follower_index}/_ccr/follow", params.merge(body: body, action: "follow", rest_api: "ccr") |
| # ccr.follow("follower_index", { leader_index: "leader_index", remote_cluster: "leader" }) | ||
| # | ||
| # See https://www.elastic.co/guide/en/elasticsearch/reference/current/ccr-put-follow.html | ||
| def follow(follower_index, body, params = {}) |
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.
Should we at least check to make sure the following index was created here?
{ "follow_index_created" : true }
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 think that is on the user (elastomer) to confirm the index was created.