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

Always return metadata version if metadata is requested #37674

Merged
merged 3 commits into from
Jan 23, 2019
Merged

Always return metadata version if metadata is requested #37674

merged 3 commits into from
Jan 23, 2019

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jan 21, 2019

If the indices of a ClusterStateRequest are specified, we fail to
include the cluster state metadata version in the response.

@dnhatn
Always return cluster state metadata version
66baeb8 
If the indices of a ClusterStateRequest are specified, we fail to
include the cluster state metadata version in the response.
@dnhatn dnhatn added >bug v7.0.0 :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v6.7.0 v6.6.1 labels Jan 21, 2019
@dnhatn dnhatn requested a review from ywelsch January 21, 2019 21:40
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

ywelsch
ywelsch suggested changes Jan 22, 2019
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

There are more oddities here, namely that the metadata settings, templates etc. are not returned when explicit indices are requested. Before we make changes to this API, we need to fully understand the implications though, how this API is being used.
What was the motivation for this PR?

.../src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java Outdated
@@ -152,6 +152,7 @@ private void buildResponse(final ClusterStateRequest request,
MetaData.Builder mdBuilder = MetaData.builder();
mdBuilder.clusterUUID(currentState.metaData().clusterUUID());
mdBuilder.coordinationMetaData(currentState.coordinationMetaData());
mdBuilder.version(currentState.metaData().version());
Copy link
Contributor

Choose a reason for hiding this comment

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

why always return it, even if the metadata was not requested?

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 feel that the "version" belongs the metadata group of the metadata just like "clusterUUID".

Copy link
Contributor

Choose a reason for hiding this comment

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

The cluster uuid is exceptional (because it used in many places for identification purposes), the metadata version not so much (imo), as there is also the cluster state version (that is persisted starting in 7.0). I think we should move mdBuilder.version(currentState.metaData().version()); below if (request.indices().length > 0) {

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 am ok with your suggestion :).

@dnhatn
Copy link
Member Author

dnhatn commented Jan 22, 2019

What was the motivation for this PR?

The proposal in #37633 does not work as expected because the returned metadata version is always 0.

@dnhatn dnhatn changed the title Always return cluster state metadata version Always return metadata version if metadata is requested Jan 22, 2019
@dnhatn
Copy link
Member Author

dnhatn commented Jan 22, 2019

run elasticsearch-ci/1

ywelsch
ywelsch approved these changes Jan 22, 2019
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

@dnhatn
Copy link
Member Author

dnhatn commented Jan 22, 2019 •
edited
Loading

@ywelsch Thanks for reviewing this carefully.

There are more oddities here, namely that the metadata settings, templates etc. are not returned when explicit indices are requested.

I took a look at the Metadata. It may be ok not to return settings and templates if the indices are specified because the settings and templates can be retrieved via /_cluster/settings and /_templates APIs respectively. One option that I can see is to provide flags (indices, settings, templates) to allow users to specify the data that they want to get from Metadata. I can do it in a follow-up. WDYT?

@ywelsch
Copy link
Contributor

ywelsch commented Jan 23, 2019

One option that I can see is to provide flags (indices, settings, templates) to allow users to specify the data that they want to get from Metadata. I can do it in a follow-up. WDYT?

Let's leave it at this for now, and only introduce once there is a concrete need for it.

@dnhatn dnhatn merged commit 6a98383 into elastic:master Jan 23, 2019
@dnhatn dnhatn deleted the fix-metadata-version branch January 23, 2019 15:24
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 23, 2019
@jasontedor
Merge branch 'master' into optimize-warning-head-de-duplication
94b4687 
* master:
  Liberalize StreamOutput#writeStringList (elastic#37768)
  Add PersistentTasksClusterService::unassignPersistentTask method (elastic#37576)
  Tests: disable testRandomGeoCollectionQuery on tiny polygons (elastic#37579)
  Use ILM for Watcher history deletion (elastic#37443)
  Make sure PutMappingRequest accepts content types other than JSON. (elastic#37720)
  Retry ILM steps that fail due to SnapshotInProgressException (elastic#37624)
  Use disassociate in preference to deassociate (elastic#37704)
  Delete Redundant RoutingServiceTests (elastic#37750)
  Always return metadata version if metadata is requested (elastic#37674)
dnhatn added a commit that referenced this pull request Jan 23, 2019
@dnhatn
Always return metadata version if metadata is requested (#37674)
6551e5d 
If the indices of a ClusterStateRequest are specified, we fail to
include the cluster state metadata version in the response.

Relates  #37633
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 24, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into sync-retention-leases
37380dd 
* elastic/master: (85 commits)
  Use explicit version for build-tools in example plugin integ tests (elastic#37792)
  Change `rational` to `saturation` in script_score (elastic#37766)
  Deprecate types in get field mapping API (elastic#37667)
  Add ability to listen to group of affix settings (elastic#37679)
  Ensure changes requests return the latest mapping version (elastic#37633)
  Make Minio Setup more Reliable (elastic#37747)
  Liberalize StreamOutput#writeStringList (elastic#37768)
  Add PersistentTasksClusterService::unassignPersistentTask method (elastic#37576)
  Tests: disable testRandomGeoCollectionQuery on tiny polygons (elastic#37579)
  Use ILM for Watcher history deletion (elastic#37443)
  Make sure PutMappingRequest accepts content types other than JSON. (elastic#37720)
  Retry ILM steps that fail due to SnapshotInProgressException (elastic#37624)
  Use disassociate in preference to deassociate (elastic#37704)
  Delete Redundant RoutingServiceTests (elastic#37750)
  Always return metadata version if metadata is requested (elastic#37674)
  [TEST] Mute MlMappingsUpgradeIT testMappingsUpgrade
  Streamline skip_unavailable handling (elastic#37672)
  Only bootstrap and elect node in current voting configuration (elastic#37712)
  Ensure either success or failure path for SearchOperationListener is called (elastic#37467)
  Target only specific index in update settings test
  ...
@dnhatn dnhatn mentioned this pull request Jan 24, 2019
Merged
dnhatn added a commit that referenced this pull request Jan 26, 2019
@dnhatn
Always return metadata version if metadata is requested (#37674)
ec243f2 
If the indices of a ClusterStateRequest are specified, we fail to
include the cluster state metadata version in the response.

Relates  #37633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywelsch ywelsch ywelsch approved these changes

Assignees
No one assigned
Labels
>bug :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v6.6.1 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
@dnhatn @elasticmachine @ywelsch @colings86