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 GetTasksResponse TaskFailure deserialization #727

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Bfindlay
Copy link
Contributor

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.

dblock
dblock previously approved these changes Nov 15, 2023
@reta
Copy link
Collaborator

reta commented Nov 15, 2023

@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
Copy link
Member

dblock commented Nov 15, 2023

@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
Copy link
Contributor Author

@reta Interesting 👀 let me take a look.

@Bfindlay
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Collaborator

reta commented Nov 16, 2023

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
Copy link
Contributor Author

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.

dblock
dblock previously approved these changes Nov 18, 2023
@Bfindlay
Copy link
Contributor Author

@reta PR updated to return JsonData for both properties 👍

@reta
Copy link
Collaborator

reta commented Nov 23, 2023

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

reta commented Nov 24, 2023

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 Bfindlay marked this pull request as draft November 26, 2023 10:30
@Bfindlay
Copy link
Contributor Author

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.

".opensearch-observability",
".opendistro_security",
".plugins-ml-config",
".tasks"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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]>
@@ -38,6 +38,8 @@
import org.opensearch.common.settings.Settings;

public abstract class AbstractIndicesClientIT extends OpenSearchJavaClientTestCase {

@Test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove @Test, it should not be needed


public abstract class AbstractTasksIT extends OpenSearchJavaClientTestCase {

@Test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove @Test

public abstract class AbstractTasksIT extends OpenSearchJavaClientTestCase {

@Test
public void getTasks_waitForCompletionFalse_jsonDataStatusCanBeDeserialized() throws IOException, InterruptedException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

import org.opensearch.client.opensearch.tasks.GetTasksResponse;
import org.opensearch.client.opensearch.tasks.Status;

public class GetTasksResponseTest extends Assert {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

"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
Copy link
Collaborator

@reta reta Nov 27, 2023

Choose a reason for hiding this comment

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

Why system indices should be excluded?

@Bfindlay
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

reta commented Dec 17, 2023

@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
Copy link
Contributor

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

@reta
Copy link
Collaborator

reta commented Apr 18, 2024

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

@hapeka72
Copy link

Any news here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants