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.

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

Fix .tasks index strict mapping: parent_id should be parent_task_id #48393

Merged
merged 3 commits into from
Oct 25, 2019

Conversation

javanna
Copy link
Member

@javanna javanna commented Oct 23, 2019

The .tasks index has mappings that's strictly defined. parent_task_id
was defined as parent_id though which would cause an exception in case
a task is persisted that has a parent task id set.

While at it, a couple of compiler warnings were addressed and a test
request builder was removed in favour of using its corresponding request.

The .tasks index has mappings that's strictly defined. `parent_task_id`
was defined as `parent_id` though which would cause an exception in case
a task is persisted that has a parent task id set.

While at it, a couple of compiler warnings were addressed and a test
request builder was removed in favour of using its corresponding request.
@javanna javanna added >bug :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. v8.0.0 v7.5.0 labels Oct 23, 2019
@javanna javanna requested a review from jimczi October 23, 2019 12:55
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Task Management)

@javanna
Copy link
Member Author

javanna commented Oct 23, 2019

run elasticsearch-ci/1

@javanna
Copy link
Member Author

javanna commented Oct 23, 2019

run elasticsearch-ci/default-distro

@javanna
Copy link
Member Author

javanna commented Oct 23, 2019

run elasticsearch-ci/bwc

Copy link
Contributor

@henningandersen henningandersen 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 looking into this @javanna. I left a single drive-by comment.

@@ -19,7 +19,7 @@
"id": {
"type": "long"
},
"parent_id": {
"parent_task_id": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to increment the version in TaskResultsService.TASK_RESULT_MAPPING_VERSION in order to get the new mapping applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, I was not aware of that. Do you know if there are tests around applying the updated mappings?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think not, since there would at least be no verification that the parent_task_id field is in the index after/during a rolling upgrade. I also seem to remember that some of the test frameworks delete the .tasks index between each test (but not sure if it applies to all upgrade tests).

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I don't see a case where a parent task would be set on a task that is persisted but this would save some time if this is needed in the future so LGTM.

@javanna
Copy link
Member Author

javanna commented Oct 25, 2019

@elasticmachine update branch

@javanna
Copy link
Member Author

javanna commented Oct 25, 2019

run elasticsearch-ci/packaging-sample-matrix

@javanna javanna merged commit 77a7bb6 into elastic:master Oct 25, 2019
javanna added a commit that referenced this pull request Oct 25, 2019
…48393)

* Fix .tasks index strict mapping: parent_id should be parent_task_id

The .tasks index has mappings that's strictly defined. `parent_task_id`
was defined as `parent_id` though which would cause an exception in case
a task is persisted that has a parent task id set.

While at it, a couple of compiler warnings were addressed and a test
request builder was removed in favour of using its corresponding request.

* increment version
romseygeek added a commit that referenced this pull request Dec 20, 2019
The built-in task index mapping has a version field in its metadata, so that
the TaskResultsService can check to see if it needs to update mappings when
a new task result is stored. #48393 updated this version in TaskResultsService
but omitted to change the version in the mapping itself, so a mapping update
is applied every time a new task result is stored.

This commit updates the mapping version so that it corresponds to the version
in TaskResultsService.
romseygeek added a commit that referenced this pull request Dec 20, 2019
The built-in task index mapping has a version field in its metadata, so that
the TaskResultsService can check to see if it needs to update mappings when
a new task result is stored. #48393 updated this version in TaskResultsService
but omitted to change the version in the mapping itself, so a mapping update
is applied every time a new task result is stored.

This commit updates the mapping version so that it corresponds to the version
in TaskResultsService.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Coordination/Task Management Issues for anything around the Tasks API - both persistent and node level. v7.5.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants