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

Clean up duplicate follow config parameter code #37688

Merged
merged 13 commits into from
Feb 5, 2019
Merged

Clean up duplicate follow config parameter code #37688

merged 13 commits into from
Feb 5, 2019

Conversation

martijnvg
Copy link
Member

Introduced FollowParameters class that put follow, resume follow,
put auto follow pattern requests and follow info response classes reuse.

The FollowParameters class had the fields, getters etc. for the common parameters
that all these APIs have. Also binary and xcontent serialization /
parsing is handled by this class.

The follow, resume follow, put auto follow pattern request classes originally
used optional non primitive fields, so FollowParameters has that too and the follow info api can handle that now too.

The follow, resume follow, put auto follow pattern request classes now all have an inner Body class, that encapsulates what they accept as input. Something like this had to be introduced, because the Request classes already extend a base action class.

This is an attempt to centralize the common ccr follow parameters. In a follow up I like to explore changing AutoFollowPattern and ShardFollowTask to make using of FollowParameters. This is trickier because both classes use immutable fields (which I think we do want to keep) and ShardFollowTask uses primitive fields. However I think we can do something like, making FollowParameters abstract and have a base class that uses non final fields and a base class that uses final fields.

This PR doesn't reduce the LOC that much, but I think the great win here is that the follow config is now less duplicated across the code base.

@martijnvg
Cleaned up duplicate follow config parameter handling
bdd66cb 
Introduced FollowParameters class that put follow, resume follow,
put auto follow pattern requests and follow info response classes reuse.

The FollowParameters class had the fields, getters etc. for the common parameters
that all these APIs have.  Also binary and xcontent serialization /
parsing is handled by this class.

The follow, resume follow, put auto follow pattern request classes originally
used optional non primitive fields, so FollowParameters has that too and the follow info api can handle that now too.
@martijnvg martijnvg added >non-issue v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.7.0 labels Jan 22, 2019
@dnhatn
Copy link
Member

dnhatn commented Feb 2, 2019

@martijnvg This is a great step. I am sorry for not looking this earlier.

The follow, resume follow, put auto follow pattern request classes now all have an inner Body class, that encapsulates what they accept as input. Something like this had to be introduced, because the Request classes already extend a base action class.

I know you're much more familiar than me in the XContent world, but can we avoid introducing the Body class? Can we have two fields in the ResumeFollowAction#Request: followerIndex and followerParameters instead?

@martijnvg
Copy link
Member Author

Can we have two fields in the ResumeFollowAction#Request: followerIndex and followerParameters instead?

Yes, that should be possible. Let me try and change that.

@martijnvg
Merge remote-tracking branch 'es/master' into ccr_follow_config_cleanup
5800597
@martijnvg
Removed Body class from ResumeFollowAction.Request class.
0ea6547 
The followerIndex field can in production only be specified via
the url path. If it is also specified via the request body then
it must have the same value as is specified in the url path. This
option only existed to xcontent testing. However the AbstractSerializingTestCase
base class now also supports createXContextTestInstance() to provide
a different test instance when testing xcontent, so allowing followerIndex
to be specified via the request body is no longer needed.

By moving the followerIndex field from Body to ResumeFollowAction.Request
class and not allowing the followerIndex field to be specified via
the request body the Body class is redundant and can be removed. The
ResumeFollowAction.Request class can then directly use the
FollowParameters class.

For consistency I also removed the ability to specified followerIndex
in the put follow api and the name in put auto follow pattern api via
the request body.
@martijnvg
Copy link
Member Author

@dnhatn I've updated the PR. Removing the body class from the resume follow api, was relatively straightforward, because the followerIndex field was only allowed to be specified via the request body for testing purposes. See the commit message in latest commit.

dnhatn
dnhatn approved these changes Feb 4, 2019
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 some super minor comments but this is a really good change. Thanks @martijnvg. Can we also remove the duplicate in AutoFollowMetadata.java in a follow-up?

...n/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java Show resolved Hide resolved
...ck/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutFollowAction.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowParameters.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowParameters.java Outdated Show resolved Hide resolved
@martijnvg
Copy link
Member Author

Can we also remove the duplicate in AutoFollowMetadata.java in a follow-up?

Yes, I was planning to that in a follow up anyway in order to keep this PR reviewable.

@martijnvg
Copy link
Member Author

@elasticmachine please run elasticsearch-ci/2

@martijnvg martijnvg merged commit 0beb3c9 into elastic:master Feb 5, 2019
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Feb 5, 2019
@martijnvg
Clean up duplicate follow config parameter code (elastic#37688)
07dab70 
Introduced FollowParameters class that put follow, resume follow,
put auto follow pattern requests and follow info response classes reuse.

The FollowParameters class had the fields, getters etc. for the common parameters
that all these APIs have.  Also binary and xcontent serialization /
parsing is handled by this class.

The follow, resume follow, put auto follow pattern request classes originally
used optional non primitive fields, so FollowParameters has that too and the follow info api can handle that now too.

Also the followerIndex field can in production only be specified via
the url path. If it is also specified via the request body then
it must have the same value as is specified in the url path. This
option only existed to xcontent testing. However the AbstractSerializingTestCase
base class now also supports createXContextTestInstance() to provide
a different test instance when testing xcontent, so allowing followerIndex
to be specified via the request body is no longer needed.

By moving the followerIndex field from Body to ResumeFollowAction.Request
class and not allowing the followerIndex field to be specified via
the request body the Body class is redundant and can be removed. The
ResumeFollowAction.Request class can then directly use the
FollowParameters class.

For consistency I also removed the ability to specified followerIndex
in the put follow api and the name in put auto follow pattern api via
the request body.
@martijnvg martijnvg mentioned this pull request Feb 5, 2019
Merged
martijnvg added a commit that referenced this pull request Feb 5, 2019
@martijnvg
Clean up duplicate follow config parameter code (#37688) (#38443)
571cf52 
Introduced FollowParameters class that put follow, resume follow,
put auto follow pattern requests and follow info response classes reuse.

The FollowParameters class had the fields, getters etc. for the common parameters
that all these APIs have.  Also binary and xcontent serialization /
parsing is handled by this class.

The follow, resume follow, put auto follow pattern request classes originally
used optional non primitive fields, so FollowParameters has that too and the follow info api can handle that now too.

Also the followerIndex field can in production only be specified via
the url path. If it is also specified via the request body then
it must have the same value as is specified in the url path. This
option only existed to xcontent testing. However the AbstractSerializingTestCase
base class now also supports createXContextTestInstance() to provide
a different test instance when testing xcontent, so allowing followerIndex
to be specified via the request body is no longer needed.

By moving the followerIndex field from Body to ResumeFollowAction.Request
class and not allowing the followerIndex field to be specified via
the request body the Body class is redundant and can be removed. The
ResumeFollowAction.Request class can then directly use the
FollowParameters class.

For consistency I also removed the ability to specified followerIndex
in the put follow api and the name in put auto follow pattern api via
the request body.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 6, 2019
@jasontedor
Merge branch '6.x' into retention-leases-recovery-6.x
3344768 
* 6.x: (25 commits)
  Backport of types removal for Put/Get index templates (elastic#38465)
  Add support for API keys to access Elasticsearch (elastic#38291) (elastic#38399)
  Deprecate support for internal versioning for concurrency control (elastic#38451)
  Deprecate types in rollover index API (elastic#38389) (elastic#38458)
  Add typless client side GetIndexRequest calls and response class (elastic#38422)
  [ML] Report index unavailable instead of waiting for lazy node (elastic#38444)
  await fix CurtIT#testIndex until elastic#38451 is merged (elastic#38466)
  Update ilm-api.asciidoc, point to REMOVE policy (elastic#38235) (elastic#38464)
  SQL: Fix esType for DATETIME/DATE and INTERVALS (elastic#38179)
  Clean up duplicate follow config parameter code (elastic#37688) (elastic#38443)
  Deprecation check for No Master Block setting (elastic#38383)
  Bubble-up exceptions from scheduler (elastic#38441)
  Lift retention lease expiration to index shard (elastic#38391)
  Deprecate maxRetryTimeout in RestClient and increase default value (elastic#38425)
  Update Rollup Caps to allow unknown fields (elastic#38446)
  Backport of elastic#38411: `if_seq_no` and `if_primary_term` parameters aren't wired correctly in REST Client's CRUD API
  Support unknown fields in ingest pipeline map configuration (elastic#38429)
  SQL: Implement CURRENT_DATE (elastic#38175)
  Backport changes to the release notes script. (elastic#38346)
  Fix ILM explain response to allow unknown fields (elastic#38363)
  ...
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

@jasontedor jasontedor Awaiting requested review from jasontedor

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

Successfully merging this pull request may close these issues.
4 participants
@martijnvg @dnhatn @colings86 @danielmitterdorfer