Correctly Set query status #2227

kaituo · 2023-10-05T20:26:32Z

Description

Previously, the query status was set to SUCCESS only when the EMR-S job status was 'SUCCESS'. However, for index queries, even when the EMR Job Run State is 'RUNNING', the result should indicate success. This PR addresses and resolves this inconsistency.

Tests done:

Manual verification: Created a skipping index and confirmed the async query result is marked 'successful' instead of 'running'.
Updated relevant unit tests.

Issues Resolved

#2214

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

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.

Previously, the query status was set to SUCCESS only when the EMR-S job status was 'SUCCESS'. However, for index queries, even when the EMR Job Run State is 'RUNNING', the result should indicate success. This PR addresses and resolves this inconsistency. Tests done: * Manual verification: Created a skipping index and confirmed the async query result is marked 'successful' instead of 'running'. * Updated relevant unit tests. Signed-off-by: Kaituo Li <kaituo@amazon.com>

codecov · 2023-10-05T20:45:53Z

Codecov Report

Merging #2227 (d224e5e) into main (a83482d) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #2227   +/-   ##
=========================================
  Coverage     96.36%   96.36%           
+ Complexity     4727     4726    -1     
=========================================
  Files           439      439           
  Lines         12686    12688    +2     
  Branches        872      871    -1     
=========================================
+ Hits          12225    12227    +2     
  Misses          452      452           
  Partials          9        9

Flag	Coverage Δ
sql-engine	`96.36% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...rch/sql/spark/dispatcher/SparkQueryDispatcher.java	`100.00% <100.00%> (ø)`
...sql/spark/response/JobExecutionResponseReader.java	`100.00% <100.00%> (ø)`

penghuo · 2023-10-05T20:45:57Z

spark/src/main/java/org/opensearch/sql/spark/response/JobExecutionResponseReader.java

    try {
      searchResponseActionFuture = client.search(searchRequest);
    } catch (Exception e) {
+      // if there is no result index (e.g., EMR-S hasn't created the index yet), we return empty
+      // json
+      if (e instanceof IndexNotFoundException) {


nit: we could catch IndexNotFoundException?

Probably build is failing because we are missing unit tests for this piece of code.

@vamsi-amazon yeah, adding one now

@penghuo good catch. changed.

penghuo · 2023-10-05T20:47:26Z

spark/src/main/java/org/opensearch/sql/spark/dispatcher/SparkQueryDispatcher.java

      result.put(STATUS_FIELD, status);

      // If items have ERROR_FIELD, use it; otherwise, set empty string
      String error = items.optString(ERROR_FIELD, "");
      result.put(ERROR_FIELD, error);
    } else {
+      // make call to EMR Serverless when related result index documents are not available


nice! it is help reduce query latency.

penghuo · 2023-10-05T20:48:46Z

spark/src/main/java/org/opensearch/sql/spark/dispatcher/SparkQueryDispatcher.java

+    // That a job is running does not mean the status is running. For example, index/streaming Query
+    // is a
+    // long-running job which runs forever. But we need to return success from the result index
+    // immediately.
    if (result.has(DATA_FIELD)) {


should we check status field? is it possible no data filed in result?

as we discussed, we can assume status exist within data

penghuo · 2023-10-05T20:49:58Z

spark/src/main/java/org/opensearch/sql/spark/dispatcher/SparkQueryDispatcher.java

    if (result.has(DATA_FIELD)) {
      JSONObject items = result.getJSONObject(DATA_FIELD);

      // If items have STATUS_FIELD, use it; otherwise, use jobState
-      String status = items.optString(STATUS_FIELD, jobState);
+      String status = items.optString(STATUS_FIELD, JobRunState.FAILED.toString());


in which case, result has data but does not has status field?

theoretically it is not possible. I provide a default here in case we get exception when it is not there.

Signed-off-by: Kaituo Li <kaituo@amazon.com>

vamsimanohar · 2023-10-05T22:35:16Z

docs/user/ppl/admin/connectors/s3glue_connector.rst

@@ -14,10 +14,18 @@ S3Glue Connector
 Introduction
 ============

+Properties in DataSource Configuration


This should be below introduction right. Can we move this to Glue Connector Properties in DataSource Configuration Section.

* Correctly Set query status Previously, the query status was set to SUCCESS only when the EMR-S job status was 'SUCCESS'. However, for index queries, even when the EMR Job Run State is 'RUNNING', the result should indicate success. This PR addresses and resolves this inconsistency. Tests done: * Manual verification: Created a skipping index and confirmed the async query result is marked 'successful' instead of 'running'. * Updated relevant unit tests. Signed-off-by: Kaituo Li <kaituo@amazon.com> * add unit test Signed-off-by: Kaituo Li <kaituo@amazon.com> --------- Signed-off-by: Kaituo Li <kaituo@amazon.com> (cherry picked from commit cd9d768) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Correctly Set query status Previously, the query status was set to SUCCESS only when the EMR-S job status was 'SUCCESS'. However, for index queries, even when the EMR Job Run State is 'RUNNING', the result should indicate success. This PR addresses and resolves this inconsistency. Tests done: * Manual verification: Created a skipping index and confirmed the async query result is marked 'successful' instead of 'running'. * Updated relevant unit tests. * add unit test --------- (cherry picked from commit cd9d768) Signed-off-by: Kaituo Li <kaituo@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Correctly Set query status Previously, the query status was set to SUCCESS only when the EMR-S job status was 'SUCCESS'. However, for index queries, even when the EMR Job Run State is 'RUNNING', the result should indicate success. This PR addresses and resolves this inconsistency. Tests done: * Manual verification: Created a skipping index and confirmed the async query result is marked 'successful' instead of 'running'. * Updated relevant unit tests. * add unit test --------- (cherry picked from commit cd9d768) Signed-off-by: Kaituo Li <kaituo@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> (cherry picked from commit 00ae8ce) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Correctly Set query status Previously, the query status was set to SUCCESS only when the EMR-S job status was 'SUCCESS'. However, for index queries, even when the EMR Job Run State is 'RUNNING', the result should indicate success. This PR addresses and resolves this inconsistency. Tests done: * Manual verification: Created a skipping index and confirmed the async query result is marked 'successful' instead of 'running'. * Updated relevant unit tests. * add unit test --------- (cherry picked from commit cd9d768) (cherry picked from commit 00ae8ce) Signed-off-by: Kaituo Li <kaituo@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

kaituo requested review from pjfitzgibbons, ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsimanohar, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto and GumpacG as code owners October 5, 2023 20:26

vamsimanohar previously approved these changes Oct 5, 2023

View reviewed changes

vamsimanohar assigned kaituo Oct 5, 2023

vamsimanohar added bug Something isn't working backport 2.x Flint v2.11.0 Issues targeting release v2.11.0 labels Oct 5, 2023

penghuo reviewed Oct 5, 2023

View reviewed changes

penghuo previously approved these changes Oct 5, 2023

View reviewed changes

add unit test

d224e5e

Signed-off-by: Kaituo Li <kaituo@amazon.com>

kaituo dismissed stale reviews from penghuo and vamsimanohar via d224e5e October 5, 2023 21:07

penghuo approved these changes Oct 5, 2023

View reviewed changes

vamsimanohar approved these changes Oct 5, 2023

View reviewed changes

vamsimanohar reviewed Oct 5, 2023

View reviewed changes

penghuo merged commit cd9d768 into opensearch-project:main Oct 5, 2023

opensearch-trigger-bot bot mentioned this pull request Oct 5, 2023

[Backport 2.x] Correctly Set query status #2231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly Set query status #2227

Correctly Set query status #2227

kaituo commented Oct 5, 2023

codecov bot commented Oct 5, 2023 •

edited

Loading

penghuo Oct 5, 2023

vamsimanohar Oct 5, 2023

kaituo Oct 5, 2023

kaituo Oct 5, 2023

penghuo Oct 5, 2023

penghuo Oct 5, 2023

kaituo Oct 5, 2023

penghuo Oct 5, 2023

kaituo Oct 5, 2023

vamsimanohar Oct 5, 2023

Correctly Set query status #2227

Correctly Set query status #2227

Conversation

kaituo commented Oct 5, 2023

Description

Issues Resolved

Check List

codecov bot commented Oct 5, 2023 • edited Loading

Codecov Report

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

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

codecov bot commented Oct 5, 2023 •

edited

Loading