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.

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

Remove dependency on doc versions #29906

Merged
merged 24 commits into from Feb 5, 2019

Conversation

Projects
None yet
@spalger
Copy link
Member

spalger commented Feb 2, 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.

  • 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.

  • 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.

  • 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, 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. 52d890f

  • 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.

@spalger spalger added the WIP label Feb 2, 2019

@spalger spalger requested review from elastic/kibana-canvas as code owners Feb 2, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

elasticmachine commented Feb 2, 2019

spalger added some commits Feb 2, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

elasticmachine commented Feb 2, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

elasticmachine commented Feb 2, 2019

@spalger spalger changed the title Remove dependency on ES version Remove dependency on doc versions Feb 3, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

elasticmachine commented Feb 3, 2019

@elasticmachine

This comment was marked as outdated.

Copy link

elasticmachine commented Feb 3, 2019

spalger added some commits Feb 4, 2019

Revert "disable --bail"
This reverts commit 35343fc.

@spalger spalger added the review label Feb 4, 2019

@njd5475 njd5475 self-requested a review Feb 4, 2019

@joelgriffith

This comment has been minimized.

Copy link
Contributor

joelgriffith commented Feb 4, 2019

💥 LGTM.

@joelgriffith
Copy link
Contributor

joelgriffith left a comment

Pulled and ran locally, looks good 👍

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Feb 4, 2019

@mikecote
Copy link
Contributor

mikecote left a comment

LGTM 👍

@njd5475

njd5475 approved these changes Feb 4, 2019

Copy link
Contributor

njd5475 left a comment

LGTM

@elasticmachine

This comment was marked as off-topic.

Copy link

elasticmachine commented Feb 5, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Feb 5, 2019

@spalger spalger merged commit b4725b7 into elastic:master Feb 5, 2019

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

@zube zube bot added [zube]: Done and removed [zube]: In Review labels Feb 5, 2019

@spalger spalger removed the v6.7.0 label Feb 5, 2019

spalger added a commit to spalger/kibana that referenced this pull request Feb 5, 2019

Remove dependency on doc versions (elastic#29906)
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.
@walterra

This comment has been minimized.

Copy link
Contributor

walterra commented Feb 5, 2019

You're right we haven't been using version in ML in that context, LGTM!

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Feb 12, 2019

@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?

@spalger

This comment has been minimized.

Copy link
Member Author

spalger commented Feb 13, 2019

@sqren best option would be to have it covered by a functional test, I'm kind of surprised that I missed that.

spalger added a commit that referenced this pull request Feb 14, 2019

[6.x] Remove dependency on doc versions (#29906) (#30036)
Backports the following commits to 6.x:
 - Remove dependency on doc versions  (#29906)

@kobelb kobelb referenced this pull request Feb 19, 2019

Merged

[Saved Objects] Add hidden types to savedobjects #28722

9 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment