Fix GetTasksResponse TaskFailure deserialization #727

Bfindlay · 2023-11-15T11:18:37Z

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

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

reta · 2023-11-15T15:09:56Z

@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] elastic/elasticsearch-specification#2250

dblock · 2023-11-15T15:22:57Z

@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] elastic/elasticsearch-specification#2250

Good eyes. @Bfindlay

Bfindlay · 2023-11-15T21:56:54Z

@reta Interesting 👀 let me take a look.

Bfindlay · 2023-11-16T07:33:11Z

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.

Bfindlay · 2023-11-16T09:51:08Z

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

reta · 2023-11-16T13:03:36Z

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

Bfindlay · 2023-11-16T13:08:51Z

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.

Bfindlay · 2023-11-23T05:37:49Z

@reta PR updated to return JsonData for both properties 👍

reta · 2023-11-23T21:26:39Z

@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

Bfindlay · 2023-11-24T02:10:09Z

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

reta · 2023-11-24T13:20:01Z

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

Bfindlay · 2023-11-26T22:18:00Z

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.

Bfindlay · 2023-11-27T08:53:06Z

...t/src/test/java/org/opensearch/client/opensearch/integTest/OpenSearchJavaClientTestCase.java

+        ".opensearch-observability",
+        ".opendistro_security",
+        ".plugins-ml-config",
+        ".tasks"


This was causing the pipeline failure 🤦 so I have added the .tasks system index here.

Signed-off-by: bfindlay <[email protected]> update changelog Signed-off-by: bfindlay <[email protected]> revert Info change Signed-off-by: bfindlay <[email protected]> change response to JsonData type Signed-off-by: bfindlay <[email protected]> fix spottless check Signed-off-by: bfindlay <[email protected]> Update status to JsonData Signed-off-by: bfindlay <[email protected]> apply spotless check Signed-off-by: bfindlay <[email protected]> add itest for JsonData deserialization Signed-off-by: bfindlay <[email protected]> update itest Signed-off-by: bfindlay <[email protected]> update itest Signed-off-by: bfindlay <[email protected]> fix itest Signed-off-by: bfindlay <[email protected]> fix pipeline failure Signed-off-by: bfindlay <[email protected]> exclude system indices from cat itest Signed-off-by: bfindlay <[email protected]>

Signed-off-by: bfindlay <[email protected]>

reta · 2023-11-27T15:13:10Z

...client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractIndicesClientIT.java

@@ -38,6 +38,8 @@
 import org.opensearch.common.settings.Settings;

 public abstract class AbstractIndicesClientIT extends OpenSearchJavaClientTestCase {
+
+    @Test


Please remove @Test, it should not be needed

reta · 2023-11-27T15:13:30Z

java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractTasksIT.java

+
+public abstract class AbstractTasksIT extends OpenSearchJavaClientTestCase {
+
+    @Test


Please remove @Test

reta · 2023-11-27T15:13:53Z

java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractTasksIT.java

+public abstract class AbstractTasksIT extends OpenSearchJavaClientTestCase {
+
+    @Test
+    public void getTasks_waitForCompletionFalse_jsonDataStatusCanBeDeserialized() throws IOException, InterruptedException {


Suggested change

public void getTasks_waitForCompletionFalse_jsonDataStatusCanBeDeserialized() throws IOException, InterruptedException {

public void testGetTasksAndNoWaitForCompletionFals() throws IOException, InterruptedException {

reta · 2023-11-27T15:39:55Z

java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractHighlightIT.java

@@ -105,7 +105,9 @@ public void testAnalyzerOffset() throws Exception {
    private List<Map<String, List<String>>> highlightQuery(String query, Function<Highlight.Builder, ObjectBuilder<Highlight>> fn)
        throws IOException {
        SearchResponse<Article> response = javaClient().search(
-            s -> s.query(q -> q.simpleQueryString(sqs -> sqs.fields("title", "content", "author").query(query))).highlight(fn),
+            s -> s.index("*,-.*") // exclude system indices


I believe the system indices are excluded from search (and also none should much this query?)

reta · 2023-11-27T15:40:54Z

java-client/src/test/java/org/opensearch/client/opensearch/core/GetTasksResponseTest.java

+import org.opensearch.client.opensearch.tasks.GetTasksResponse;
+import org.opensearch.client.opensearch.tasks.Status;
+
+public class GetTasksResponseTest extends Assert {


Do we need this test now since we have integration ones?

reta · 2023-11-27T15:41:43Z

java-client/src/test/java/org/opensearch/client/opensearch/integTest/AbstractCatClientIT.java

-                    "index,shard,type,stage,source_host,source_node,"
-                        + "target_host,target_node,repository,snapshot,files,files_recovered,files_percent,files_total"
-                ).bytes(Bytes.Bytes)
+                r -> r.index("*,-.*") // exclude system indices


Why system indices should be excluded?

Bfindlay · 2023-11-27T21:38:04Z

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

Bfindlay · 2023-12-13T09:51:35Z

@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 · 2023-12-17T21:57:53Z

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

uriofferup · 2024-04-18T17:10:47Z

Hello @reta and @Bfindlay , where are we with this ? can I help to solve the issue ?.
Thank you

reta · 2024-04-18T17:16:06Z

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

Bfindlay requested review from reta, Bukhtawar, dblock, szczepanczykd, madhusudhankonda, saratvemulapalli and VachaShah as code owners November 15, 2023 11:18

Bfindlay mentioned this pull request Nov 15, 2023

[BUG] UnexpectedJsonEventException reading task that failed #666

Open

dblock previously approved these changes Nov 15, 2023

View reviewed changes

Bfindlay dismissed dblock’s stale review via 7bd4900 November 16, 2023 09:40

dblock previously approved these changes Nov 18, 2023

View reviewed changes

Bfindlay dismissed dblock’s stale review via aa9f389 November 26, 2023 08:51

Bfindlay marked this pull request as draft November 26, 2023 10:30

Bfindlay commented Nov 27, 2023

View reviewed changes

Bfindlay force-pushed the bfindlay/issue-666 branch from 486a0b1 to fbaee9d Compare November 27, 2023 09:10

Bfindlay added 2 commits November 27, 2023 20:13

spotless check

7ce55cd

Signed-off-by: bfindlay <[email protected]>

fix itest system index for highlight

d5c062f

Signed-off-by: bfindlay <[email protected]>

fix system index access for index itest

865c41a

Signed-off-by: bfindlay <[email protected]>

reta reviewed Nov 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GetTasksResponse TaskFailure deserialization #727

Fix GetTasksResponse TaskFailure deserialization #727

Bfindlay commented Nov 15, 2023

reta commented Nov 15, 2023

dblock commented Nov 15, 2023

Bfindlay commented Nov 15, 2023

Bfindlay commented Nov 16, 2023

Bfindlay commented Nov 16, 2023

reta commented Nov 16, 2023

Bfindlay commented Nov 16, 2023

Bfindlay commented Nov 23, 2023

reta commented Nov 23, 2023

Bfindlay commented Nov 24, 2023

reta commented Nov 24, 2023 •

edited

Loading

Bfindlay commented Nov 26, 2023

Bfindlay Nov 27, 2023

reta Nov 27, 2023

reta Nov 27, 2023

reta Nov 27, 2023

reta Nov 27, 2023

reta Nov 27, 2023

reta Nov 27, 2023 •

edited

Loading

Bfindlay commented Nov 27, 2023

Bfindlay commented Dec 13, 2023

reta commented Dec 17, 2023

uriofferup commented Apr 18, 2024

reta commented Apr 18, 2024


		public abstract class AbstractTasksIT extends OpenSearchJavaClientTestCase {

		@Test

	public void getTasks_waitForCompletionFalse_jsonDataStatusCanBeDeserialized() throws IOException, InterruptedException {
	public void testGetTasksAndNoWaitForCompletionFals() throws IOException, InterruptedException {

Fix GetTasksResponse TaskFailure deserialization #727

Are you sure you want to change the base?

Fix GetTasksResponse TaskFailure deserialization #727

Conversation

Bfindlay commented Nov 15, 2023

Description

Issues Resolved

reta commented Nov 15, 2023

dblock commented Nov 15, 2023

Bfindlay commented Nov 15, 2023

Bfindlay commented Nov 16, 2023

Bfindlay commented Nov 16, 2023

reta commented Nov 16, 2023

Bfindlay commented Nov 16, 2023

Bfindlay commented Nov 23, 2023

reta commented Nov 23, 2023

Bfindlay commented Nov 24, 2023

reta commented Nov 24, 2023 • edited Loading

Bfindlay commented Nov 26, 2023

Bfindlay Nov 27, 2023

Choose a reason for hiding this comment

reta Nov 27, 2023

Choose a reason for hiding this comment

reta Nov 27, 2023

Choose a reason for hiding this comment

reta Nov 27, 2023

Choose a reason for hiding this comment

reta Nov 27, 2023

Choose a reason for hiding this comment

reta Nov 27, 2023

Choose a reason for hiding this comment

reta Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Bfindlay commented Nov 27, 2023

Bfindlay commented Dec 13, 2023

reta commented Dec 17, 2023

uriofferup commented Apr 18, 2024

reta commented Apr 18, 2024

reta commented Nov 24, 2023 •

edited

Loading

reta Nov 27, 2023 •

edited

Loading