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

Move persistent task registrations to core #30755

Merged
merged 4 commits into from
May 24, 2018
Merged

Move persistent task registrations to core #30755

merged 4 commits into from
May 24, 2018

Conversation

droberts195
Copy link
Contributor

@droberts195 droberts195 commented May 21, 2018 •
edited
Loading

Persistent tasks was moved from X-Pack to core in #28455.
However, registration of the named writables and named
X-content was left in X-Pack.

This change moves the registration of the named writables
and named X-content into core. Additionally, the persistent
task actions are no longer registered in the X-Pack client
plugin, as they are already registered in ActionModule.

@droberts195
Move persistent task registrations to core
e03c4f2 
Persistent tasks was moved from X-Pack to core in elastic#28455.
However, registration of the named writables and named
X-content was left in X-Pack.

This change moves the registration of the named writables
and named X-content into core.  Additionally, the persistent
task actions are no longer registered in the X-Pack client
plugin, as they are already registered in ActionModule.
@droberts195 droberts195 added >non-issue review :Distributed/Task Management Issues for anything around the Tasks API - both persistent and node level. v7.0.0 v6.3.0 v6.4.0 labels May 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@droberts195
Copy link
Contributor Author

I marked this as a >non-issue because presumably moving persistent tasks to core will be release-noted under #28455.

jasontedor
jasontedor approved these changes May 21, 2018
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM yet I would like @martijnvg and @tlrx to take a look too.

@jasontedor jasontedor requested a review from tlrx May 21, 2018 13:23
droberts195
droberts195 commented May 21, 2018
View reviewed changes
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java
StartPersistentTaskAction.INSTANCE,
UpdatePersistentTaskStatusAction.INSTANCE,
RemovePersistentTaskAction.INSTANCE,
CompletionPersistentTaskAction.INSTANCE,
Copy link
Contributor Author

@droberts195 droberts195 May 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think these are redundant because the same four actions are already registered in ActionModule.

martijnvg
martijnvg approved these changes May 22, 2018
View reviewed changes
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thanks for catching this @droberts195. I left a comment, but other than this LGTM.

server/src/main/java/org/elasticsearch/common/network/NetworkModule.java Outdated
@@ -94,6 +98,18 @@
new NamedWriteableRegistry.Entry(Task.Status.class, ReplicationTask.Status.NAME, ReplicationTask.Status::new));
namedWriteables.add(
new NamedWriteableRegistry.Entry(Task.Status.class, RawTaskStatus.NAME, RawTaskStatus::new));
namedWriteables.add(
Copy link
Member

Choose a reason for hiding this comment

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

I think that ClusterModule is a better place to register these classes, because this class already has the majority (I think all of them) default custom metadata registrations and in my mind NetworkModule is more for real network things like http and transport.

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

tlrx
tlrx approved these changes May 22, 2018
View reviewed changes
Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

Thanks @droberts195

server/src/main/java/org/elasticsearch/common/network/NetworkModule.java Outdated
@@ -94,6 +98,18 @@
new NamedWriteableRegistry.Entry(Task.Status.class, ReplicationTask.Status.NAME, ReplicationTask.Status::new));
namedWriteables.add(
new NamedWriteableRegistry.Entry(Task.Status.class, RawTaskStatus.NAME, RawTaskStatus::new));
namedWriteables.add(
Copy link
Member

Choose a reason for hiding this comment

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

I agree.

@droberts195 droberts195 merged commit aafcd85 into elastic:master May 24, 2018
@droberts195 droberts195 deleted the move_persistent_task_registrations_to_core branch May 24, 2018 08:17
droberts195 added a commit that referenced this pull request May 24, 2018
@droberts195
Move persistent task registrations to core (#30755)
914929b 
Persistent tasks was moved from X-Pack to core in #28455.
However, registration of the named writables and named
X-content was left in X-Pack.

This change moves the registration of the named writables
and named X-content into core.  Additionally, the persistent
task actions are no longer registered in the X-Pack client
plugin, as they are already registered in ActionModule.
droberts195 added a commit that referenced this pull request May 24, 2018
@droberts195
Move persistent task registrations to core (#30755)
5bac2af 
Persistent tasks was moved from X-Pack to core in #28455.
However, registration of the named writables and named
X-content was left in X-Pack.

This change moves the registration of the named writables
and named X-content into core.  Additionally, the persistent
task actions are no longer registered in the X-Pack client
plugin, as they are already registered in ActionModule.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 24, 2018
@jasontedor
Merge branch 'master' into verify-official-plugin-signature
9d40715 
* master:
  Update the version checks around ip_range bucket keys, now that the change was backported.
  Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges
  Use geohash cell instead of just a corner in geo_bounding_box (elastic#30698)
  Limit user to single concurrent auth per realm (elastic#30794)
  [Tests] Move templated _rank_eval tests (elastic#30679)
  Security: fix dynamic mapping updates with aliases (elastic#30787)
  Ensure that ip_range aggregations always return bucket keys. (elastic#30701)
  Use remote client in TransportFieldCapsAction (elastic#30838)
  Move Watcher versioning setting to meta field (elastic#30832)
  [Docs] Explain incomplete dates in range queries (elastic#30689)
  Move persistent task registrations to core (elastic#30755)
  Decouple ClusterStateTaskListener & ClusterApplier (elastic#30809)
  Send client headers from TransportClient (elastic#30803)
  Packaging: Ensure upgrade_is_oss flag file is always deleted (elastic#30732)
  Force stable file modes for built packages (elastic#30823)
  [DOCS] Fixes typos in security settings
  Fix GeoShapeQueryBuilder serialization after backport
martijnvg added a commit that referenced this pull request May 25, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
e6dd414 
* es/master:
  Move score script context from SearchScript to its own class (#30816)
  Fix bad version check writing Repository nodes (#30846)
  [docs] explainer for java packaging tests (#30825)
  Remove Throwable usage from transport modules (#30845)
  REST high-level client: add put ingest pipeline API (#30793)
  Update the version checks around ip_range bucket keys, now that the change was backported.
  Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges
  Use geohash cell instead of just a corner in geo_bounding_box (#30698)
  Limit user to single concurrent auth per realm (#30794)
  [Tests] Move templated _rank_eval tests (#30679)
  Security: fix dynamic mapping updates with aliases (#30787)
  Ensure that ip_range aggregations always return bucket keys. (#30701)
  Use remote client in TransportFieldCapsAction (#30838)
  Move Watcher versioning setting to meta field (#30832)
  [Docs] Explain incomplete dates in range queries (#30689)
  Move persistent task registrations to core (#30755)
  Decouple ClusterStateTaskListener & ClusterApplier (#30809)
  Send client headers from TransportClient (#30803)
  Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732)
  Force stable file modes for built packages (#30823)
martijnvg added a commit that referenced this pull request May 25, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
545ed87 
* es/6.x:
  Move score script context from SearchScript to its own class (#30816)
  Fix bad version check writing Repository nodes (#30846)
  Modify state of VerifyRepositoryResponse for bwc (#30762)
  QA: Fix tribe tests when running default zip
  Use remote client in TransportFieldCapsAction (#30838)
  Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges
  Ensure that ip_range aggregations always return bucket keys. (#30701)
  Limit user to single concurrent auth per realm (#30794)
  Security: fix dynamic mapping updates with aliases (#30787)
  [Tests] Move templated _rank_eval tests (#30679)
  Move Watcher versioning setting to meta field (#30832)
  Restore "Add more yaml tests for get alias API " (#30814)
  Send client headers from TransportClient (#30803)
  [Docs] Explain incomplete dates in range queries (#30689)
  Move persistent task registrations to core (#30755)
  Decouple ClusterStateTaskListener & ClusterApplier (#30809)
  Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732)
  Force stable file modes for built packages (#30823)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martijnvg martijnvg martijnvg approved these changes

@tlrx tlrx tlrx approved these changes

@jasontedor jasontedor jasontedor approved these changes

Assignees
No one assigned
Labels
:Distributed/Task Management Issues for anything around the Tasks API - both persistent and node level. >non-issue v6.3.0 v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@droberts195 @elasticmachine @martijnvg @tlrx @jasontedor @colings86