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.

Sign up for GitHub

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

Jump to bottom

[CCR] Add get auto follow pattern api #33849

Merged
merged 8 commits into from
Sep 24, 2018
Merged

[CCR] Add get auto follow pattern api #33849

merged 8 commits into from
Sep 24, 2018

Conversation

martijnvg
Copy link
Member

Relates to #33007

@martijnvg martijnvg added review :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features labels Sep 19, 2018
@martijnvg martijnvg requested a review from dnhatn September 19, 2018 09:15
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

dnhatn
dnhatn reviewed Sep 22, 2018
View reviewed changes
Copy link
Member

@dnhatn dnhatn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@martijnvg This looks great to me, but I am wondering if we should not enforce the GET request always require one clusterAlias. Should we return all auto-patterns if the clusterAlias is omitted? Thank you!

@martijnvg
Copy link
Member Author

@dnhatn Thanks for reviewing. I made the clusterAlias parameter optional and in that case all auto follow patterns are returned.

dnhatn
dnhatn reviewed Sep 23, 2018
View reviewed changes
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

I left a comment to discuss.

...n/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/GetAutoFollowPatternAction.java Outdated

public static class Response extends ActionResponse implements ToXContentObject {

private List<AutoFollowPattern> autoFollowPatterns;
Copy link
Member

Choose a reason for hiding this comment

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

Should we return a map of "clusterAlias" to auto-follow patterns instead of a list?

Copy link
Member Author

@martijnvg martijnvg Sep 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, if we add clusterAlias to AutoFollowPattern then we can return a map. I will add that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed this commit to let the auto follow patterns to be returned as a map: 96bf947

dnhatn
dnhatn approved these changes Sep 24, 2018
View reviewed changes
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @martijnvg.

@martijnvg martijnvg merged commit 2795ef5 into elastic:master Sep 24, 2018
martijnvg added a commit that referenced this pull request Sep 24, 2018
@martijnvg
[CCR] Add get auto follow pattern api (#33849)
c6ea38f 
Relates to #33007
kcm pushed a commit that referenced this pull request Oct 30, 2018
@martijnvg @kcm
[CCR] Add get auto follow pattern api (#33849)
7320b92 
Relates to #33007
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnhatn dnhatn dnhatn approved these changes

Assignees
No one assigned
Labels
:Distributed Indexing/CCR Issues around the Cross Cluster State Replication features >non-issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martijnvg @elasticmachine @dnhatn @jasontedor @tomcallahan