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

Make version field names more meaningful #35334

Merged
merged 2 commits into from
Nov 7, 2018
Merged

Make version field names more meaningful #35334

merged 2 commits into from
Nov 7, 2018

Conversation

alpar-t
Copy link
Contributor

@alpar-t alpar-t commented Nov 7, 2018 •
edited
Loading

Replaces the recently added version.build_version with version.qualified in the main response.
We refer to it as version_qualified in the doc tests.

@alpar-t alpar-t requested a review from nik9000 November 7, 2018 13:17
@alpar-t alpar-t added the :Delivery/Build Build or test infrastructure label Nov 7, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@alpar-t
Copy link
Contributor Author

alpar-t commented Nov 7, 2018

CC @elastic/es-clients

nik9000
nik9000 approved these changes Nov 7, 2018
View reviewed changes
Copy link
Member

@nik9000 nik9000 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 for doing it!

@alpar-t alpar-t merged commit 5ae0319 into elastic:master Nov 7, 2018
@alpar-t alpar-t deleted the remove_build_version branch November 7, 2018 16:36
@jasontedor
Copy link
Member

jasontedor commented Nov 7, 2018 •
edited
Loading

I am sorry I am catching this after merge, but I am not onboard with this change. Why are we not reporting the qualifier in the version.number field as we did previously?

@alpar-t
Copy link
Contributor Author

alpar-t commented Nov 7, 2018

@jasontedor that was my initial line of thinking but then I looked at how snapshot is treated and taught of being consistent with that. The server does no longer grasp the concept of a qualifier, the only reason it's not fully cleaned up is that we still have some versions that have it. Server does know about the version as presented by the build which has both qualifier and snapshot. Are you ok with having version.number potentially contain both snapshot and qualifier ? This would imply that Version.fromString would ignore them when creating a Version object.
@nik9000 do you like that proposal ?

jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Nov 8, 2018
@jasontedor
Merge branch 'master' into pr/35195
2a65635 
* master: (24 commits)
  Replicate index settings to followers (elastic#35089)
  Rename RealmConfig.globalSettings() to settings() (elastic#35330)
  [TEST] Cleanup FileUserPasswdStoreTests (elastic#35329)
  Scripting: Add back lookup vars in score script (elastic#34833)
  watcher: Fix integration tests to ensure correct start/stop of Watcher (elastic#35271)
  Remove ALL shard check in CheckShrinkReadyStep (elastic#35346)
  Use soft-deleted docs to resolve strategy for engine operation (elastic#35230)
  [ILM] Check shard and relocation status in AllocationRoutedStep (elastic#35316)
  Ignore date ranges containing 'now' when pre-processing a percolator query (elastic#35160)
  Add a frozen engine implementation (elastic#34357)
  Put a fake allocation id on allocate stale primary command (elastic#34140)
  [CCR] Enforce auto follow pattern name restrictions (elastic#35197)
  [ILM] rolling upgrade tests (elastic#35328)
  [ML] Add Missing data checking class (elastic#35310)
  Apply `ignore_throttled` also to concrete indices (elastic#35335)
  Make version field names more meaningful  (elastic#35334)
  [CCR] Added HLRC support for pause follow API (elastic#35216)
  [Docs] Improve Convert Processor description (elastic#35280)
  [Painless] Removes extraneous compile method (elastic#35323)
  [CCR] Fail with a better error if leader index is red (elastic#35298)
  ...
@nik9000
Copy link
Member

nik9000 commented Nov 8, 2018

We're not reporting the qualifier in the number because Version.java doesn't understand the qualifier any more. I think Jason's right though. We'd be better off returning it the same way we did before. We could ignore the qualifier when parsing the version in MainResponse. Or we could make a high-level-rest-client version of MainResponse that doesn't parse the number into a Version.

@nik9000
Copy link
Member

nik9000 commented Nov 8, 2018

I don't think we should modify Version.fromString, instead, if we want to keep Version on the high level rest client's response object, we should trim those fields from the string before we throw them into it.

@alpar-t alpar-t mentioned this pull request Nov 9, 2018
Merged
pgomulka pushed a commit to pgomulka/elasticsearch that referenced this pull request Nov 13, 2018
@alpar-t @pgomulka
Make version field names more meaningful (elastic#35334)
c5ee6f7 
* Consolidate the name of the qualified build version

* Field name in response should not be redundant
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees
No one assigned
Labels
:Delivery/Build Build or test infrastructure >non-issue Team:Delivery Meta label for Delivery team v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alpar-t @elasticmachine @jasontedor @nik9000 @mark-vieira @jimczi