Repository navigation
Remove dependency on doc versions - #29906
Merged
spalger merged 24 commits intoFeb 5, 2019
Merged
Conversation
added 8 commits
February 1, 2019 23:56
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
added 2 commits
February 3, 2019 08:47
This comment has been minimized.
This comment has been minimized.
added 2 commits
February 3, 2019 18:12
This reverts commit 35343fc.
tylersmalley
reviewed
Feb 4, 2019
tylersmalley
reviewed
Feb 4, 2019
added 2 commits
February 4, 2019 17:20
tylersmalley
approved these changes
Feb 5, 2019
This comment has been minimized.
This comment has been minimized.
Contributor
💚 Build Succeeded |
spalger
pushed a commit
to spalger/kibana
that referenced
this pull request
Feb 5, 2019
See elastic/elasticsearch#38254 Using the `version` parameter to implement optimistic concurrency is not going to be supported in 7.0, so we need to replace our usage of document version with the new `_seq_no` and `_primary_term` parameters. These fields are returned in the same way that `_version` was returned on all read/write requests except for search, where it needs to be requested by sending `seq_no_primary_term: true` in the body of the search request. These parameters are sent back to Elasticsearch on write requests with the `if_seq_no` and `if_primary_term` parameters, and are functionally equivalent to sending a `version` in a write request before elastic/elasticsearch#38254. To make these updates I searched the code base for uses of a `version` and `_version`, then triaged each usage, so I'm fairly confident that I got everything but it's possible something slipped through the cracks, so if you know of any usage of the document version field please help me out by double checking that I converted it. - [x] **Saved Objects**: @elastic/kibana-platform, @elastic/es-security - for BWC and ergonomics the `version` provided by the Saved Objects client/API was not removed, it was converted from a number to a string whose value is `base64(json([_seq_no, _primary_term]))`. This allows the Saved Objects API and its consumers to remain mostly unmodified, as long as the underlying value in the version field is irrelevant. This was the case for all usages in Kibana, only thing that needed updating was tests and TS types. - [x] **Reporting/esqueue**: @joelgriffith, @tsullivan - the version parameter was used here specifically for implementing optimistic concurrency, and since its usage was contained within the esqueue module I just updated it to use the new `_seq_no` and `_primary_term` fields. - [x] **Task Manager**: @tsullivan @njd5475 - Like esqueue this module uses version for optimistic concurrency but the usage is contained with the module so I just updated it to use, store, and request the `_seq_no` and `_primary_term` fields. - [ ] **ML**: @elastic/ml-ui - Best I could tell the only "version" in the ML code refers to the stack version, elastic@077245f - [ ] **Beats CM**: @elastic/beats - Looks like the references to `_version` in the code is only in the types but not in the code itself. I updated the types to use `_seq_no` and `_primary_term`, and their camelCase equivalents where appropriate. I did find a method that used one of the types referencing version but when investigating its usage it seemed the only consumer of that method was itself so i removed it. elastic@52d890f - [x] **Spaces (tests)**: @elastic/kibana-security - The spaces test helpers use saved objects with versions in a number of places, so I updated them to use the new string versions where the version was predictable, and removed the assertion on version where it wasn't. We test the version in the saved objects code so this should be fine.
Contributor
|
You're right we haven't been using |
1 of 7 tasks
Contributor
|
@spalger This change caused some things to go awry with APM Getting Started flow (#30651). But not to worry! :) We have a PR ready with a fix #30796. Can you verify it is the correct approach? Btw. Is there something we could have done to avoid this? Eg. add a test or somehow made it clear we depend on the saved objects API? |
Contributor
Author
|
@sqren best option would be to have it covered by a functional test, I'm kind of surprised that I missed that. |
spalger
pushed a commit
that referenced
this pull request
Feb 14, 2019
9 tasks done
weltenwort
added a commit
that referenced
this pull request
Mar 11, 2019
patrykkopycinski
pushed a commit
to patrykkopycinski/kibana
that referenced
this pull request
May 6, 2026
See elastic/elasticsearch#38254 Using the `version` parameter to implement optimistic concurrency is not going to be supported in 7.0, so we need to replace our usage of document version with the new `_seq_no` and `_primary_term` parameters. These fields are returned in the same way that `_version` was returned on all read/write requests except for search, where it needs to be requested by sending `seq_no_primary_term: true` in the body of the search request. These parameters are sent back to Elasticsearch on write requests with the `if_seq_no` and `if_primary_term` parameters, and are functionally equivalent to sending a `version` in a write request before elastic/elasticsearch#38254. To make these updates I searched the code base for uses of a `version` and `_version`, then triaged each usage, so I'm fairly confident that I got everything but it's possible something slipped through the cracks, so if you know of any usage of the document version field please help me out by double checking that I converted it. - [x] **Saved Objects**: @elastic/kibana-platform, @elastic/es-security - for BWC and ergonomics the `version` provided by the Saved Objects client/API was not removed, it was converted from a number to a string whose value is `base64(json([_seq_no, _primary_term]))`. This allows the Saved Objects API and its consumers to remain mostly unmodified, as long as the underlying value in the version field is irrelevant. This was the case for all usages in Kibana, only thing that needed updating was tests and TS types. - [x] **Reporting/esqueue**: @joelgriffith, @tsullivan - the version parameter was used here specifically for implementing optimistic concurrency, and since its usage was contained within the esqueue module I just updated it to use the new `_seq_no` and `_primary_term` fields. - [x] **Task Manager**: @tsullivan @njd5475 - Like esqueue this module uses version for optimistic concurrency but the usage is contained with the module so I just updated it to use, store, and request the `_seq_no` and `_primary_term` fields. - [ ] **ML**: @elastic/ml-ui - Best I could tell the only "version" in the ML code refers to the stack version, elastic@077245f - [ ] **Beats CM**: @elastic/beats - Looks like the references to `_version` in the code is only in the types but not in the code itself. I updated the types to use `_seq_no` and `_primary_term`, and their camelCase equivalents where appropriate. I did find a method that used one of the types referencing version but when investigating its usage it seemed the only consumer of that method was itself so i removed it. elastic@52d890f - [x] **Spaces (tests)**: @elastic/kibana-security - The spaces test helpers use saved objects with versions in a number of places, so I updated them to use the new string versions where the version was predictable, and removed the assertion on version where it wasn't. We test the version in the saved objects code so this should be fine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See elastic/elasticsearch#38254
Using the
versionparameter to implement optimistic concurrency is not going to be supported in 7.0, so we need to replace our usage of document version with the new_seq_noand_primary_termparameters. These fields are returned in the same way that_versionwas returned on all read/write requests except for search, where it needs to be requested by sendingseq_no_primary_term: truein the body of the search request. These parameters are sent back to Elasticsearch on write requests with theif_seq_noandif_primary_termparameters, and are functionally equivalent to sending aversionin a write request before elastic/elasticsearch#38254.To make these updates I searched the code base for uses of a
versionand_version, then triaged each usage, so I'm fairly confident that I got everything but it's possible something slipped through the cracks, so if you know of any usage of the document version field please help me out by double checking that I converted it.Saved Objects: @elastic/kibana-platform, @elastic/es-security - for BWC and ergonomics the
versionprovided by the Saved Objects client/API was not removed, it was converted from a number to a string whose value isbase64(json([_seq_no, _primary_term])). This allows the Saved Objects API and its consumers to remain mostly unmodified, as long as the underlying value in the version field is irrelevant. This was the case for all usages in Kibana, only thing that needed updating was tests and TS types.Reporting/esqueue: @joelgriffith, @tsullivan - the version parameter was used here specifically for implementing optimistic concurrency, and since its usage was contained within the esqueue module I just updated it to use the new
_seq_noand_primary_termfields.Task Manager: @tsullivan @njd5475 - Like esqueue this module uses version for optimistic concurrency but the usage is contained with the module so I just updated it to use, store, and request the
_seq_noand_primary_termfields.ML: @elastic/ml-ui - Best I could tell the only "version" in the ML code refers to the stack version, 077245f
Beats CM: @elastic/beats - Looks like the references to
_versionin the code is only in the types but not in the code itself. I updated the types to use_seq_noand_primary_term, and their camelCase equivalents where appropriate. I did find a method that used one of the types referencing version but when investigating its usage it seemed the only consumer of that method was itself so i removed it. 52d890fSpaces (tests): @elastic/kibana-security - The spaces test helpers use saved objects with versions in a number of places, so I updated them to use the new string versions where the version was predictable, and removed the assertion on version where it wasn't. We test the version in the saved objects code so this should be fine.