Catch task description error #12834

dblock · 2024-03-21T15:35:50Z

Description

Alternative to (just the first two commits) to #12831.

A user that enables trace-level logging with OpenSearch will run into a serialization error on geo tasks that they otherwise would not run into. It comes from here. This also happens in a test, and #12783 attempts to ignore the failed serialization error. This PR always returns a task description.

Reproduce with

./gradlew ':server:test' --tests "org.opensearch.search.geo.GeoPointShapeQueryTests.testQueryLinearRing" -Dtests.opensearch.logger.level=TRACE

Related Issues

Closes #11688
Closes #12324

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

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

reta · 2024-03-21T17:16:22Z

@reta Do you see any bad unintended consequences here?

No, I don't, thanks @andrross , I also feel like this is the direction we should be taking, I've suggested a bit more wider fix here #12834 (comment)

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

dblock · 2024-03-21T19:14:38Z

@reta implemented your suggested approach + test

server/src/main/java/org/opensearch/action/search/SearchRequest.java

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

andrross · 2024-03-21T19:57:39Z

A bit of a side note, given that a linear ring is a line string with some special constraints (start and end point must be the same), I wonder why ShapeType.LINEARRING was introduced? It seems like LinearRing could have just been a child of Line (which it currently is) with a constructor enforcing its special constraint, but otherwise could have delegated all other logic to the parent Line class.

github-actions · 2024-03-21T20:04:47Z

✅ Gradle check result for 316479d: SUCCESS

codecov · 2024-03-21T20:07:22Z

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 71.31%. Comparing base (b15cb0c) to head (71d3f02).
Report is 71 commits behind head on main.

Files	Patch %	Lines
...va/org/opensearch/action/search/SearchRequest.java	66.66%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12834      +/-   ##
============================================
- Coverage     71.42%   71.31%   -0.11%     
- Complexity    59978    60170     +192     
============================================
  Files          4985     5011      +26     
  Lines        282275   283664    +1389     
  Branches      40946    41117     +171     
============================================
+ Hits         201603   202294     +691     
- Misses        63999    64595     +596     
- Partials      16673    16775     +102

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

reta · 2024-03-21T20:20:54Z

A bit of a side note, given that a linear ring is a line string with some special constraints (start and end point must be the same), I wonder why ShapeType.LINEARRING was introduced?

Hard to say to be fair, wanna take a look? ;-)

github-actions · 2024-03-21T20:49:39Z

❕ Gradle check result for 71d3f02: UNSTABLE

TEST FAILURES:

      3 org.opensearch.cluster.MinimumClusterManagerNodesIT.testThreeNodesNoClusterManagerBlock

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

reta · 2024-03-21T21:04:20Z

It's all yours @dblock !

opensearch-trigger-bot · 2024-03-22T00:36:05Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-12834-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0c956e81373c9d5878710c1bafe471431000a64f
# Push it to GitHub
git push --set-upstream origin backport/backport-12834-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-12834-to-2.x.

reta · 2024-03-22T00:36:39Z

@dblock could you please backport to 2.x manually? thank you!

shwetathareja · 2024-03-22T03:53:47Z

server/src/main/java/org/opensearch/action/search/SearchRequest.java

+            sb.append("source[");
+            try {
+                sb.append(source.toString(FORMAT_PARAMS));
+            } catch (final OpenSearchException ex) {


@reta / @dblock / @andrross : This is not right. buildDescription() shouldn't swallow exception. Anything unexpected should fail the request as early as possible. Earlier, IOException was wrapped and thrown and now that also gets swallowed. Why an object which is throwing UnsupportedOperationException not handled properly and reached till here at all? In order to fix an user behavior (which may not be an issue, as actual request may not reach till this point as well, unless someone has tested an actual user rest request and confirmed the behavior?). Tomorrow, somebody would wrap NPE also as OpenSearchException and start swallowing here as apparently buildDescription should succeed for some reason.

Why an object which is throwing UnsupportedOperationException not handled properly and reached till here at all?

@shwetathareja There are few levels this exception will be raised and it will be handled properly, it is not swallowed. The issue that the description of the request (serialization of the POJO) - the string representation - triggers the problem first, even before the request is processed (and validated). So the suggested fix here is to provide partial description of the request that will eventually fail for exactly the same cause. This is what tests are verifying - the request fails with UnsupportedOperationException

Specifically in the request path the request is read, then traced (including its description, which throws exception), then validated & al. If we switch the order of things and decide whether the request should be accepted before tracing it, then tracing will fail to show what the request being validated was.

When the rest request will come, first the request has to be parsed/ deserialized, shouldn't it fail there, rather fail during serialization flow which comes after that.

I tried indexing and search linearring shape type with trace logging enabled on OpenSearch 2.11 cluster.
trace logging enabled

"logger": { "org": { "opensearch": { "tasks": "TRACE" } } }

Below are the errors, I got:

During Indexing:

"caused_by": { "type": "illegal_argument_exception", "reason": "No enum constant org.opensearch.geometry.ShapeType.LINERRING" }

During search

"caused_by": { "type": "illegal_argument_exception", "reason": "unknown geo_shape [linearring]" }

The error during search is as expected from the parsing flow:

OpenSearch/server/src/main/java/org/opensearch/common/geo/parsers/GeoJsonParser.java

Line 85 in 4010ff1

final GeoShapeType type = GeoShapeType.forName(subParser.text());

Do you have an example of a rest request which will reach the serialization error ( from .getDescription()) which is fixed in the code above or the validation error you mentioned.

It is not an end-to-end test, The test simply testing a particular flow for GeoShapeQueryBuilder and expecting an exception.

The builder could validate the supported shapes in AbstractGeometryQueryBuilder when creating the POJO itself.

I already replied to that here #12834 (comment)

I think the ideal option is @andrross 's idea to get rid of LinearRing if possible - this is supporting abstraction

@dblock : Are you reverting this change?

No, I am still convinced that the implementation merged here is better than what we had. Today tasks can fail to getDescription() and the better place to guard it is in the code we added it in. If we get rid of LinearRing, then we don't need to protect from failed serialization.

Once we remove the Linearring, the only exception, you are taking care in getDescription() is IOException which we shouldn't catch and swallow even for getDescription() to work. I don't think there is need for this exception handling considering it has always been like this, not impacting any other tests/ flow yet.

I agree re:once we remove the Linearring. I opened #12910, I'll try to get to it.

* Always return a task description even when it cannot be serialized. Signed-off-by: dblock <[email protected]> * Expect tasks to fail. Signed-off-by: dblock <[email protected]> * Only catch exceptions when getting description. Signed-off-by: dblock <[email protected]> * Added <error: ...> to mark error more clearly. Signed-off-by: dblock <[email protected]> --------- Signed-off-by: dblock <[email protected]> (cherry picked from commit 0c956e8)

dblock · 2024-03-22T12:22:42Z

@dblock could you please backport to 2.x manually? thank you!

#12858

* Always return a task description even when it cannot be serialized. Signed-off-by: dblock <[email protected]> * Expect tasks to fail. Signed-off-by: dblock <[email protected]> * Only catch exceptions when getting description. Signed-off-by: dblock <[email protected]> * Added <error: ...> to mark error more clearly. Signed-off-by: dblock <[email protected]> --------- Signed-off-by: dblock <[email protected]> (cherry picked from commit 0c956e8)

* Always return a task description even when it cannot be serialized. Signed-off-by: dblock <[email protected]> * Expect tasks to fail. Signed-off-by: dblock <[email protected]> * Only catch exceptions when getting description. Signed-off-by: dblock <[email protected]> * Added <error: ...> to mark error more clearly. Signed-off-by: dblock <[email protected]> --------- Signed-off-by: dblock <[email protected]> Signed-off-by: Shivansh Arora <[email protected]>

Always return a task description even when it cannot be serialized.

7fc4fb0

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

dblock added the skip-changelog label Mar 21, 2024

dblock mentioned this pull request Mar 21, 2024

Implement serialization for LinearRing. #12831

Closed

8 tasks

github-actions bot added bug Something isn't working flaky-test Random test failure that succeeds on second run Geospatial Search Search query, autocomplete ...etc labels Mar 21, 2024

Only catch exceptions when getting description.

316479d

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

reta reviewed Mar 21, 2024

View reviewed changes

server/src/main/java/org/opensearch/action/search/SearchRequest.java Outdated Show resolved Hide resolved

Added <error: ...> to mark error more clearly.

71d3f02

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

reta approved these changes Mar 21, 2024

View reviewed changes

github-actions bot added the Other label Mar 21, 2024

andrross approved these changes Mar 21, 2024

View reviewed changes

andrross added the backport 2.x Backport to 2.x branch label Mar 21, 2024

reta mentioned this pull request Mar 21, 2024

Fix LinearRing serialization issue while emitting task description in… #12844

Closed

6 tasks

reta merged commit 0c956e8 into opensearch-project:main Mar 22, 2024
37 of 39 checks passed

opensearch-trigger-bot bot added the backport-failed label Mar 22, 2024

shwetathareja reviewed Mar 22, 2024

View reviewed changes

dblock mentioned this pull request Mar 22, 2024

[Backport 2.x] Catch task description error (#12834) #12858

Merged

8 tasks

dblock deleted the catch-task-description-error branch March 22, 2024 12:22

peternied mentioned this pull request Mar 22, 2024

[BUG] Some GeoShapeQueryBuilder instances cannot be toString()'d #12324

Open

dblock mentioned this pull request Mar 25, 2024

[Feature Request] Remove LinearRing #12910

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch task description error #12834

Catch task description error #12834

dblock commented Mar 21, 2024 •

edited by andrross

Loading

reta commented Mar 21, 2024 •

edited

Loading

dblock commented Mar 21, 2024

andrross commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

codecov bot commented Mar 21, 2024 •

edited

Loading

reta commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

reta commented Mar 21, 2024 •

edited

Loading

opensearch-trigger-bot bot commented Mar 22, 2024

reta commented Mar 22, 2024

shwetathareja Mar 22, 2024

reta Mar 22, 2024 •

edited

Loading

dblock Mar 22, 2024

shwetathareja Mar 22, 2024

shwetathareja Mar 22, 2024

reta Mar 22, 2024 •

edited

Loading

shwetathareja Mar 22, 2024

dblock Mar 22, 2024

shwetathareja Mar 23, 2024

dblock Mar 25, 2024

dblock commented Mar 22, 2024

Catch task description error #12834

Catch task description error #12834

Conversation

dblock commented Mar 21, 2024 • edited by andrross Loading

Description

Related Issues

Check List

reta commented Mar 21, 2024 • edited Loading

dblock commented Mar 21, 2024

andrross commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

codecov bot commented Mar 21, 2024 • edited Loading

Codecov Report

reta commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

reta commented Mar 21, 2024 • edited Loading

opensearch-trigger-bot bot commented Mar 22, 2024

reta commented Mar 22, 2024

Choose a reason for hiding this comment

reta Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Mar 22, 2024

dblock commented Mar 21, 2024 •

edited by andrross

Loading

reta commented Mar 21, 2024 •

edited

Loading

codecov bot commented Mar 21, 2024 •

edited

Loading

reta commented Mar 21, 2024 •

edited

Loading

reta Mar 22, 2024 •

edited

Loading

reta Mar 22, 2024 •

edited

Loading