Handle create index with batch FlintJob #2734

seankao-az · 2024-06-10T20:13:31Z

Description

Use batch FlintJob to handle create index query. It's already used for streaming query. This PR makes creating auto_refresh=false index use FlintJob as well.
This would resolve the bug where dropping a manual refresh index could result in cancellation of the FlintREPL job that executed the create index statement.

Alongside this change, also make the RefreshQueryHandler borrow from lease, instead of BatchQueryHandler borrowing, because the concurrent refresh job limit shouldn't apply to batch jobs in general. The behavior for refresh query would remain the same.
Making the change in this same PR because prior to this PR, there is no query that can be used to test BatchQueryHandler.

Tests

Added unit tests that cover the code path for dispatching to IndexDMLHandler, and create index queries.

End to end test

create index auto_refresh=false -> drop index
- index create should be a batch FlintJob
- drop index should
  - not cancel any job if the job running the create statement is completed
  - cancel the batch job if it isn't completed yet
create index auto_refresh=true -> alter index auto_refresh=false -> drop index
- index create should be a streaming FlintJob
- alter index should cancel the streaming job
- drop index should not further cancel any job
create index auto_refresh=false -> alter index auto_refresh=true -> drop index
- index create should be a batch FlintJob
- alter index should start a streaming job
- drop index should cancel the streaming job

Issues Resolved

[BUG] DROP INDEX cancel FlintREPL job #2689

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.

codecov · 2024-06-11T00:23:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.47%. Comparing base (cd95130) to head (4e05e17).
Report is 1 commits behind head on main.

❗ Current head 4e05e17 differs from pull request most recent head 20ae2b0

Please upload reports for the commit 20ae2b0 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #2734   +/-   ##
=========================================
  Coverage     95.47%   95.47%           
- Complexity     5193     5197    +4     
=========================================
  Files           509      509           
  Lines         14637    14639    +2     
  Branches        982      983    +1     
=========================================
+ Hits          13974    13976    +2     
  Misses          641      641           
  Partials         22       22

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

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

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

Signed-off-by: Sean Kao <[email protected]>

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

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

vmmusings · 2024-06-11T13:41:38Z

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

@@ -59,6 +60,8 @@ public String cancelJob(AsyncQueryJobMetadata asyncQueryJobMetadata) {
  @Override
  public DispatchQueryResponse submit(
      DispatchQueryRequest dispatchQueryRequest, DispatchQueryContext context) {
+    leaseManager.borrow(new LeaseRequest(JobType.BATCH, dispatchQueryRequest.getDatasource()));


Does this warrant any change in documentation?

We're moving this from parent class? The new create index job doesn't need this?

behavior doesn't change for refresh query handler.
also according to https://github.com/opensearch-project/sql/blob/main/docs/user/admin/settings.rst#pluginsqueryexecutionenginesparkrefresh_joblimit
it's intended to affect refresh job only

The new create index job doesn't need this?

DDL shouldn't be affected by the concurrent limit. Without the change this following test would fail:

sql/spark/src/test/java/org/opensearch/sql/spark/asyncquery/IndexQuerySpecTest.java

Line 848 in 3ab7851

public void concurrentRefreshJobLimitNotAppliedToDDL() {

vmmusings · 2024-06-11T13:42:28Z

Have we tested this on a working stack?

seankao-az · 2024-06-11T21:00:22Z

Have we tested this on a working stack?

yes in my test domain

Signed-off-by: Sean Kao <[email protected]>

dai-chen

As I understand, the current approach is make sure cancel job run won't happen on REPL job. I think we can discuss later how to make index operation more clear. For example, instead of separating EMR-S job, should we avoiding cancel logic for index not in refreshing state at all?

ykmr1224

Just minor comments. Those can be addressed in separate PR.

ykmr1224 · 2024-06-11T21:53:49Z

spark/src/test/java/org/opensearch/sql/spark/dispatcher/SparkQueryDispatcherTest.java

+    HashMap<String, String> tags = new HashMap<>();
+    tags.put(DATASOURCE_TAG_KEY, "my_glue");
+    tags.put(CLUSTER_NAME_TAG_KEY, TEST_CLUSTER_NAME);
+    tags.put(JOB_TYPE_TAG_KEY, JobType.BATCH.getText());


nit: consider using ImmutableMap.of (can be future refactoring)

this whole file use this same pattern. Can be a separate refactor PR for this for next release

ykmr1224 · 2024-06-11T21:56:59Z

spark/src/test/java/org/opensearch/sql/spark/dispatcher/SparkQueryDispatcherTest.java

+            new HashMap<>() {
+              {
+                put(FLINT_INDEX_STORE_AWSREGION_KEY, "eu-west-1");
+              }
+            },


nit: same as above

ykmr1224 · 2024-06-11T22:04:45Z

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

@@ -79,6 +79,9 @@ private AsyncQueryHandler getQueryHandlerForFlintExtensionQuery(
      return queryHandlerFactory.getIndexDMLHandler();
    } else if (isEligibleForStreamingQuery(indexQueryDetails)) {
      return queryHandlerFactory.getStreamingQueryHandler();
+    } else if (IndexQueryActionType.CREATE.equals(indexQueryDetails.getIndexQueryActionType())) {
+      // create should be handled by batch handler


nit: This comment might be redundant. If we keep this, should we add more context ?

will add a oneliner why this should be the case

Signed-off-by: Sean Kao <[email protected]>

* update grammar file Signed-off-by: Sean Kao <[email protected]> * batch job for create manual refresh index Signed-off-by: Sean Kao <[email protected]> * dispatcher test for index dml query Signed-off-by: Sean Kao <[email protected]> * borrow lease for refresh query, not batch Signed-off-by: Sean Kao <[email protected]> * spotlessApply Signed-off-by: Sean Kao <[email protected]> * add release note Signed-off-by: Sean Kao <[email protected]> * update comment Signed-off-by: Sean Kao <[email protected]> --------- Signed-off-by: Sean Kao <[email protected]> (cherry picked from commit b959039) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* update grammar file * batch job for create manual refresh index * dispatcher test for index dml query * borrow lease for refresh query, not batch * spotlessApply * add release note * update comment --------- (cherry picked from commit b959039) Signed-off-by: Sean Kao <[email protected]> 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>

* update grammar file Signed-off-by: Sean Kao <[email protected]> * batch job for create manual refresh index Signed-off-by: Sean Kao <[email protected]> * dispatcher test for index dml query Signed-off-by: Sean Kao <[email protected]> * borrow lease for refresh query, not batch Signed-off-by: Sean Kao <[email protected]> * spotlessApply Signed-off-by: Sean Kao <[email protected]> * add release note Signed-off-by: Sean Kao <[email protected]> * update comment Signed-off-by: Sean Kao <[email protected]> --------- Signed-off-by: Sean Kao <[email protected]> (cherry picked from commit b959039)

* update grammar file Signed-off-by: Sean Kao <[email protected]> * batch job for create manual refresh index Signed-off-by: Sean Kao <[email protected]> * dispatcher test for index dml query Signed-off-by: Sean Kao <[email protected]> * borrow lease for refresh query, not batch Signed-off-by: Sean Kao <[email protected]> * spotlessApply Signed-off-by: Sean Kao <[email protected]> * add release note Signed-off-by: Sean Kao <[email protected]> * update comment Signed-off-by: Sean Kao <[email protected]> --------- Signed-off-by: Sean Kao <[email protected]>

seankao-az mentioned this pull request Jun 10, 2024

Validate index refreshing state before cancel job #2728

Closed

6 tasks

seankao-az self-assigned this Jun 11, 2024

seankao-az added bug Something isn't working backport 2.x labels Jun 11, 2024

seankao-az marked this pull request as ready for review June 11, 2024 00:35

seankao-az requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vmmusings, Swiddis, penghuo, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG and ykmr1224 as code owners June 11, 2024 00:35

seankao-az added 5 commits June 10, 2024 18:45

update grammar file

26d7b96

Signed-off-by: Sean Kao <[email protected]>

batch job for create manual refresh index

ebe1a36

Signed-off-by: Sean Kao <[email protected]>

dispatcher test for index dml query

703cb6a

Signed-off-by: Sean Kao <[email protected]>

borrow lease for refresh query, not batch

50cf7be

Signed-off-by: Sean Kao <[email protected]>

spotlessApply

3204307

Signed-off-by: Sean Kao <[email protected]>

seankao-az force-pushed the batch-job-for-create branch from 4e05e17 to 3204307 Compare June 11, 2024 01:46

vmmusings reviewed Jun 11, 2024

View reviewed changes

spark/src/main/java/org/opensearch/sql/spark/dispatcher/SparkQueryDispatcher.java Show resolved Hide resolved

vmmusings reviewed Jun 11, 2024

View reviewed changes

spark/src/main/java/org/opensearch/sql/spark/dispatcher/BatchQueryHandler.java Show resolved Hide resolved

vmmusings reviewed Jun 11, 2024

View reviewed changes

seankao-az added 2 commits June 11, 2024 14:55

Merge branch 'main' into batch-job-for-create

443bbb0

add release note

9c90963

Signed-off-by: Sean Kao <[email protected]>

dai-chen previously approved these changes Jun 11, 2024

View reviewed changes

ykmr1224 previously approved these changes Jun 11, 2024

View reviewed changes

update comment

20ae2b0

Signed-off-by: Sean Kao <[email protected]>

seankao-az dismissed stale reviews from ykmr1224 and dai-chen via 20ae2b0 June 11, 2024 22:20

ykmr1224 approved these changes Jun 11, 2024

View reviewed changes

rupal-bq approved these changes Jun 11, 2024

View reviewed changes

seankao-az merged commit b959039 into opensearch-project:main Jun 11, 2024
20 checks passed

opensearch-trigger-bot bot mentioned this pull request Jun 11, 2024

[Backport 2.x] Handle create index with batch FlintJob #2738

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle create index with batch FlintJob #2734

Handle create index with batch FlintJob #2734

seankao-az commented Jun 10, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading

vmmusings Jun 11, 2024

dai-chen Jun 11, 2024

seankao-az Jun 11, 2024

seankao-az Jun 11, 2024 •

edited

Loading

vmmusings commented Jun 11, 2024

seankao-az commented Jun 11, 2024

dai-chen left a comment

ykmr1224 left a comment

ykmr1224 Jun 11, 2024

seankao-az Jun 11, 2024

ykmr1224 Jun 11, 2024

ykmr1224 Jun 11, 2024

seankao-az Jun 11, 2024

Handle create index with batch FlintJob #2734

Handle create index with batch FlintJob #2734

Conversation

seankao-az commented Jun 10, 2024 • edited Loading

Description

Tests

End to end test

Issues Resolved

Check List

codecov bot commented Jun 11, 2024 • 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

seankao-az Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

vmmusings commented Jun 11, 2024

seankao-az commented Jun 11, 2024

dai-chen left a comment

Choose a reason for hiding this comment

ykmr1224 left a 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

seankao-az commented Jun 10, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading

seankao-az Jun 11, 2024 •

edited

Loading