Implement serialization for LinearRing. #12831

dblock · 2024-03-21T14:15:06Z

Description

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 implements JSON serialization for LinearRing.

Reproduce with

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

Related Issues

Closes #11688.

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 <dblock@amazon.com>

dblock · 2024-03-21T14:15:57Z

LMK if you think it's better @shwetathareja @reta.

Signed-off-by: dblock <dblock@amazon.com>

github-actions · 2024-03-21T14:23:38Z

❌ Gradle check result for 098c81e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-03-21T14:38:46Z

Compatibility status:

Checks if related components are compatible with change 2271a3a

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git]

github-actions · 2024-03-21T14:43:21Z

❌ Gradle check result for 7fc4fb0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: dblock <dblock@amazon.com>

reta · 2024-03-21T14:49:24Z

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

@@ -80,7 +80,11 @@ public SearchTask(

    @Override
    public final String getDescription() {
-        return descriptionSupplier.get();
+        try {


@dblock so I've check the codebase and it seems like we have at least 4 consolidated places where UnsupportedOperationException is thrown all over the place:

GeoJson (JSON serialization)

GeometryIO (native stream serialization)

GeoShapeIndexer (indexer)

LegacyGeoShapeQueryProcessor / VectorGeoPointShapeQueryProcessor / LuceneGeometryCollector

It seems like LinearRing is special case and should not be used as a top level geometry primitive, looking a bit more on how we could fix that.

I implemented serialization for LinearRing, so that fixes (1).

andrross · 2024-03-21T15:06:33Z

I'll start with the caveat that I know nothing about GeoJson, but does this serialization conform with the GeoJSON specification? I spent a few minutes looking at this and I couldn't quite figure it out (again, I know nothing about this domain), but which of the seven allowed geometries does a linear ring serialize itself as since "linear ring" isn't one of the allowed geometries?

reta · 2024-03-21T15:15:15Z

but which of the seven allowed geometries does a linear ring serialize itself as since "linear ring" isn't one of the allowed geometries?

Thanks @andrross , I was looking for a reasons why it was not serializable in first place but it looks like this is exactly the one (https://macwright.com/2015/03/23/geojson-second-bite gives a sneak peek what a linear rings)

dblock · 2024-03-21T15:23:48Z

Maybe @nknize can take a look at this? I approached this problem from a very practical point of view: task description wants to serialize the linear ring so that the user can see what's going on in the trace, and serialization was missing. I don't know if there are negative consequences of introducing this implementation that does not confirm with the GeoJSON spec.

github-actions · 2024-03-21T15:29:59Z

❌ Gradle check result for 2271a3a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

dblock · 2024-03-21T15:30:30Z

❌ Gradle check result for 2271a3a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

org.opensearch.indices.IndicesRequestCacheIT.testCacheWithInvalidation {p0={"opensearch.experimental.feature.pluggable.caching.enabled":"false"}} #12308

andrross · 2024-03-21T15:31:49Z

I approached this problem from a very practical point of view:

This approach is completely fine for the "convert this structure to text to write into a log message" use case. The problem is the serialization is also used to create what I assume must be conforming GeoJSON potentially for communicating with other systems that expect conforming GeoJSON. Unfortunately I don't see any easy way to tease apart these use cases because the logging happens way up in the call stack while operating on very generic objects.

dblock · 2024-03-21T15:36:18Z

@andrross @reta LMK if you prefer #12834

github-actions · 2024-03-21T16:19:23Z

❌ Gradle check result for 2271a3a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-03-21T16:23:10Z

❌ Gradle check result for d08003d: TIMEOUT

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

kkmr · 2024-03-21T21:27:56Z

Looks like @rishabhmaurya also posted a solution to the issue: #12844. :-)

dblock · 2024-03-22T12:28:36Z

I'd like to leave this open for a bit, hoping someone (@nknize?) will have a strong opinion of why we should not have this serialization implementation for LinearRing.

dblock · 2024-03-22T13:08:27Z

@shwetathareja in #12834 (comment) is saying this is dead code and we should delete it.

reta · 2024-03-22T13:14:49Z

@shwetathareja in #12834 (comment) is saying this is dead code and we should delete it.

Definitely let's delete it if it is dead (but I see it being used), may be what exactly is "dead" would good to clarify

dblock · 2024-03-29T12:46:56Z

Closing in favor of #12910.

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

7fc4fb0

Signed-off-by: dblock <dblock@amazon.com>

dblock force-pushed the task-description branch from 098c81e to 7fc4fb0 Compare March 21, 2024 14:15

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

dblock added the skip-changelog label Mar 21, 2024

Expect tasks to fail.

d08003d

Signed-off-by: dblock <dblock@amazon.com>

dblock marked this pull request as draft March 21, 2024 14:30

Implement serialization for LinearRing.

2271a3a

Signed-off-by: dblock <dblock@amazon.com>

dblock changed the title ~~Always return a task description even when it cannot be serialized.~~ Implement serialization for LinearRing. Mar 21, 2024

reta reviewed Mar 21, 2024

View reviewed changes

dblock marked this pull request as ready for review March 21, 2024 15:24

dblock mentioned this pull request Mar 21, 2024

Catch task description error #12834

Merged

8 tasks

kkmr mentioned this pull request Mar 21, 2024

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

Closed

6 tasks

dblock 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

dblock closed this Mar 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement serialization for LinearRing. #12831

Implement serialization for LinearRing. #12831

dblock commented Mar 21, 2024 •

edited

Loading

dblock commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024

reta Mar 21, 2024

dblock Mar 21, 2024

andrross commented Mar 21, 2024

reta commented Mar 21, 2024 •

edited

Loading

dblock commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024

dblock commented Mar 21, 2024

andrross commented Mar 21, 2024

dblock commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

kkmr commented Mar 21, 2024

dblock commented Mar 22, 2024

dblock commented Mar 22, 2024

reta commented Mar 22, 2024

dblock commented Mar 29, 2024

Implement serialization for LinearRing. #12831

Implement serialization for LinearRing. #12831

Conversation

dblock commented Mar 21, 2024 • edited Loading

Description

Related Issues

Check List

dblock commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 21, 2024 • edited Loading

Compatibility status:

Incompatible components

Skipped components

Compatible components

github-actions bot commented Mar 21, 2024

reta Mar 21, 2024

Choose a reason for hiding this comment

dblock Mar 21, 2024

Choose a reason for hiding this comment

andrross commented Mar 21, 2024

reta commented Mar 21, 2024 • edited Loading

dblock commented Mar 21, 2024 • edited Loading

github-actions bot commented Mar 21, 2024

dblock commented Mar 21, 2024

andrross commented Mar 21, 2024

dblock commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

kkmr commented Mar 21, 2024

dblock commented Mar 22, 2024

dblock commented Mar 22, 2024

reta commented Mar 22, 2024

dblock commented Mar 29, 2024

dblock commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024 •

edited

Loading

reta commented Mar 21, 2024 •

edited

Loading

dblock commented Mar 21, 2024 •

edited

Loading