opensearch-project
Fix GetTasksResponse TaskFailure deserialization
Description
The GetTasksResponse failures is currently trying to desierialize a list of String errors, whereas in reality the response is a map which results in a deserialization error.
There was also a missing property "cancelled" on the Status object, that I have added in this PR.
Issues Resolved
https://github.com/opensearch-project/opensearch-java/issues/666
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.
@dblock @Bfindlay the Info changes look good, but Status is not , I have been looking what OpenSearch server returns and it is indeed just opaque bytes for error or response, we may follow the same fix as Elastic folks [1] (this repository is ASF v2.0 licensed, for a reference).
[1] https://github.com/elastic/elasticsearch-specification/pull/2250
@dblock @Bfindlay the
Infochanges look good, butStatusis not , I have been looking what OpenSearch server returns and it is indeed just opaque bytes for error or response, we may follow the same fix as Elastic folks [1] (this repository is ASF v2.0 licensed, for a reference).
Good eyes. @Bfindlay
Ok I see whats happening here, I will update the PR shortly.
Regarding the changes to the missing property on Info I'm going to split it out into its own PR for visibility so its not mixed in with this larger change.
@reta I have updated the GetTasksStatus.response to now return JsonData. This however raises the question, on Info.status and if it should be following the same pattern?
This however raises the question, on Info.status and if it should be following the same pattern?
Thanks @Bfindlay , I think it may need to return JsonData as well
Ah ok @reta if we want to include that change in the same PR I can probably get to it in the next few days. I think I want to double check the response sent from the opensearch cluster to double check if JsonData is correct for that field too.
@reta PR updated to return JsonData for both properties 👍
@Bfindlay it looks great, thank you, I think the test case we have is a bit isolated from the real flow, could we add the integration test for it? I could certainly help you here
@reta PR updated to return JsonData for both properties 👍
@Bfindlay it looks great, thank you, I think the test case we have is a bit isolated from the real flow, could we add the integration test for it? I could certainly help you here
I can take a look at some iTests. If you had any specific scenario ideas let me know.
I can take a look at some iTests. If you had any specific scenario ideas let me know.
We have some very limited coverage in org.opensearch.client.opensearch.integTest.AbstractNodesIT (only tasks list), would be great to add more test cases there to cover the get APIs, thank you
I can take a look at some iTests. If you had any specific scenario ideas let me know.
We have some very limited coverage in
org.opensearch.client.opensearch.integTest.AbstractNodesIT(only tasks list), would be great to add more test cases there to cover the get APIs, thank you
@reta I've added an iTest for JsonData response however facing an iTest issue in the pipeline thats not happening locally. I'll probably have time to get to this sometime over the week.
@reta I have put this back into draft, sorry for the commit spam on this one.
After adding the iTest for tasks, it has caused side effects in many other iTests due to the 'strict' configuration of the underlying http transport in regards to the OpenSearch warnings.
As it creates a .tasks system index, a lot of existing iTests are requesting all indices, or manipulating all indices which now includes the .tasks index.
Example: [this request accesses system indices: [.tasks], but in a future major version, direct access to system indices will be prevented by default] and therefore throws a WarningFailureException
I'll probably put this one on hold for a bit and can get back to it next week as this has expanded the scope quite a bit. Let me know if you have any thoughts.
@reta I might not be able to finish this one anytime soon just low on time at the moment. Will see how I go but the additional work added as side effect of the tasks iTest just added more time.
@reta I might not be able to finish this one anytime soon just low on time at the moment. Will see how I go but the additional work added as side effect of the tasks iTest just added more time.
@Bfindlay please take your time and thank you for working on this, there should be no pressure on you to finish the change as soon as possible, in any case, if you need a hand - happy to help there. Thanks again!
Hello @reta and @Bfindlay , where are we with this ? can I help to solve the issue ?. Thank you
Hello @reta and @Bfindlay , where are we with this ? can I help to solve the issue ?.
@uriofferup the issue seems to be stuck at the moment, if you have time to pick it up, would be highly appreciated, thank you