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

[CCR] Change APIs to align better with how CCR will be used #33931

Closed
martijnvg opened this issue Sep 21, 2018 · 10 comments · Fixed by #34132
Closed

[CCR] Change APIs to align better with how CCR will be used #33931

martijnvg opened this issue Sep 21, 2018 · 10 comments · Fixed by #34132
Assignees
Labels
:Distributed Indexing/CCR Issues around the Cross Cluster State Replication features

Comments

@martijnvg
Copy link
Member

The names of the current CCR APIs don't match well with what they are doing and the use cases CCR needs to support.

Based on what ccr does and the use cases it needs support there is a need for the following primitives:

  1. Create the follow index based on settings and the mapping in the leader index.
  2. Start fetching of write operations from leader and replicate into follow index.
  3. Stop the fetching of write operations from the leader and replication into follow index.
  4. Transform a follow index into a normal index. After this step it cannot become a follower index.

It should be possible to stop and than start following a leader index with the same follow index, so that parameters that control certain characteristics of the persistent tasks following leader shards can be changed to improve replication throughput.

Currently there are the following APIs:

  • create and follow api, which does 1 and 2.
  • follow api, which does 2.
  • unfollow api, which does 3.

The follow changes should be made in order to better align CCR with the supported use cases:

  1. Rename the following apis:
  • follow api > start api (/{index}/_ccr/follow > /{index}/_ccr/start)
  • unfollow api > stop api (/{index}/_ccr/unfollow > /{index}/_ccr/stop)
  1. Add a new api, named the unfollow api, that changes a follower index into a regular index, so that it can handle regular write operations. This API will require the follow index to be closed. It will remove the index.xpack.ccr.following_index setting and ccr custom index metadata in the IndexMetaData of the follow index. After this; when the index is opened then it will not longer use the FollowingEngine.
@martijnvg martijnvg added the :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features label Sep 21, 2018
@martijnvg martijnvg self-assigned this Sep 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@bleskes
Copy link
Contributor

bleskes commented Sep 21, 2018

+1

@jasontedor
Copy link
Member

jasontedor commented Sep 22, 2018

I think that follow should appear in the start and stop APIs. When I look at the endpoints /{index}/_ccr/start and /{index}/_ccr/stop I think we should be explicit about what is being started and stopped, namely following, otherwise I think the intent is not clear. Thus, I think that follow should appear in these endpoints:

  • /{index}/_ccr/start_follow
  • /{index}/_ccr/stop_follow

Additionally, I am not sure if start and stop are the right verbs. I think of start as beginning following as opposed to resuming following which this endpoint can be used for. Maybe these endpoints should be:

  • /{index}/_ccr/resume_follow
  • /{index}/_ccr/pause_follow

Next, maybe create_and_follow does not need to be so verbose. How about simply follow with the semantics that it creates the index and starts following? I realize this is slightly contrary to my point in the previous API where I felt that we needed to be explicit about what we are starting and stopping, but I don't see the need to be explicit that we are creating the index here in the verbs in the API, I think we can get away with just follow. Then we have a nice symmetry:

  • follow API
  • unfollow API
  • pause follow API
  • resume follow API

with endpoints:

  • /{index}/_ccr/follow
  • /{index}/_ccr/unfollow
  • /{index}/_ccr/pause_follow
  • /{index}/_ccr/resume_follow

What do you think @bleskes @dliappis @dnhatn @martijnvg?

@bleskes
Copy link
Contributor

bleskes commented Sep 23, 2018

I'm good with pause/resume instead of stop/start. IMO we don't need the follow about I'm ok with having it.

Re /{index}/_ccr/follow - I'm concerned with this one. I'm worried that if we in future introduce an api that does allow you to take an existing index and tell it to follow another index (where it makes senses) we'll be in trouble. Creating an index is a major event - I like it's in the api name.

@martijnvg
Copy link
Member Author

Re /{index}/_ccr/follow - I'm concerned with this one. I'm worried that if we in future introduce an api that does allow you to take an existing index and tell it to follow another index (where it makes senses) we'll be in trouble.

If we would make the follow api (which is now create and follow) only accept a http PUT and when we add the ability to turn an existing index into a follow index we also accept a http POST then would that be clear enough? The PUT variant would fail if the follow index already exists and the POST variant would fail if the follow index is missing.

I'm 👍 with pause/resume instead of stop/start and the reasoning behind it. Personally I like the symmetry that comes with the shortening from /{index}/_ccr/create_and_follow to just /{index}/_ccr/follow, but I'm ok with sticking to create and follow if others think it is clearer.

@jasontedor
Copy link
Member

@bleskes I had the same thought as you this morning regarding future APIs and had the same thought as @martijnvg as a solution to it: PUT for create and POST for an existing index. Is this satisfactory to you @bleskes?

@bleskes
Copy link
Contributor

bleskes commented Sep 23, 2018

I feel the difference between POST and PUT is too subtle and many people make mistakes. I don't feel really strongly about it so the group prefers this much better, I will go along.

martijnvg added a commit to martijnvg/elasticsearch that referenced this issue Sep 25, 2018
Renamed:
* `/{index}/_ccr/create_and_follow` to `/{index}/_ccr/follow`
* `/{index}/_ccr/unfollow` to `/{index}/_ccr/pause_follow`
* `/{index}/_ccr/follow` to `/{index}/_ccr/resume_follow`

Relates to elastic#33931
@bleskes
Copy link
Contributor

bleskes commented Sep 25, 2018

@elastic/es-clients any opinions here?

@Mpdreamz
Copy link
Member

What are the semantics behind pause/resume? If resume picks up right where pause left off then definitely agree pause/resume are better verbs then start/stop.

I agree with @boaz that the difference is a bit too subtle. If we reuse the /{index}/_ccr/follow for both PUT and POST they should at least be documented as two separate API's i.e ccr.create_and_follow.json and ccr.follow.json

There are other API's that accept both PUT and POST without changing semantics such as snapshot.create.json and the one that does index.json has active discussions to split into two explicit different API's as well.

@jasontedor
Copy link
Member

Thanks for the feedback everyone.

I feel that our proposed use of PUT and POST here is fairly conventional, and it doesn't seem the feelings against are strong (please correct me if I am reading this wrongly). We will indeed document these as separate APIs @Mpdreamz!

So let us move forward with:

PUT /{index}/_ccr/follow <-- create a new follower index
POST /{index}/_ccr/pause_follow <-- pause a follower index
POST /{index}/_ccr/resume_follow <-- resume a follower index
POST /{index}/_ccr/unfollow <-- convert a follower index to a regular index

and reserving but not currently implementing

POST /{index}/_ccr/follow <-- convert an existing index to a follower index

martijnvg added a commit that referenced this issue Sep 28, 2018
* Renamed CCR APIs

Renamed:
* `/{index}/_ccr/create_and_follow` to `/{index}/_ccr/follow`
* `/{index}/_ccr/unfollow` to `/{index}/_ccr/pause_follow`
* `/{index}/_ccr/follow` to `/{index}/_ccr/resume_follow`

Relates to #33931
martijnvg added a commit that referenced this issue Sep 28, 2018
* Renamed CCR APIs

Renamed:
* `/{index}/_ccr/create_and_follow` to `/{index}/_ccr/follow`
* `/{index}/_ccr/unfollow` to `/{index}/_ccr/pause_follow`
* `/{index}/_ccr/follow` to `/{index}/_ccr/resume_follow`

Relates to #33931
martijnvg added a commit to martijnvg/elasticsearch that referenced this issue Sep 28, 2018
The unfollow API changes a follower index into a regular index, so that it will accept write requests from clients.

For the unfollow api to work the index follow needs to be stopped and the index needs to be closed.

Closes elastic#33931
martijnvg added a commit that referenced this issue Sep 30, 2018
The unfollow API changes a follower index into a regular index, so that it will accept write requests from clients.

For the unfollow api to work the index follow needs to be stopped and the index needs to be closed.

Closes #33931
martijnvg added a commit that referenced this issue Sep 30, 2018
The unfollow API changes a follower index into a regular index, so that it will accept write requests from clients.

For the unfollow api to work the index follow needs to be stopped and the index needs to be closed.

Closes #33931
kcm pushed a commit that referenced this issue Oct 30, 2018
* Renamed CCR APIs

Renamed:
* `/{index}/_ccr/create_and_follow` to `/{index}/_ccr/follow`
* `/{index}/_ccr/unfollow` to `/{index}/_ccr/pause_follow`
* `/{index}/_ccr/follow` to `/{index}/_ccr/resume_follow`

Relates to #33931
kcm pushed a commit that referenced this issue Oct 30, 2018
The unfollow API changes a follower index into a regular index, so that it will accept write requests from clients.

For the unfollow api to work the index follow needs to be stopped and the index needs to be closed.

Closes #33931
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/CCR Issues around the Cross Cluster State Replication features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants