Flint query scheduler part 2 #2961

noCharger · 2024-09-03T08:29:39Z

Description

Abstract AsyncQueryScheduler interface, with AsyncQuerySchedulerRequest model
Inject AsyncQueryScheduler to FlintIndexOpFactory to support DROP, VACUUM, and ALTER ... WITH (auto_refresh = false)
Register OpenSearch scheduler index as system index
Refactor refresh job to support any scheduled query, decoupled scheduler job from AsyncQueryScheduler
FlintStreamingJobHouseKeeperTask uses flintIndexOpFactory so it integrates seamlessly

Local sanity test

Drop should unschedule

% curl -X POST -H "Content-Type: application/json" -d '{
  "datasource": "myglue_test",
  "lang": "sql",
  "query": "DROP MATERIALIZED VIEW myglue_test.default.count_by_status"
}' http://localhost:9200/_plugins/_async_query
{
  "queryId": "UDVuUE1jdDFqVm15Z2x1ZV90ZXN0"
}% 

                        
[2024-09-02T15:24:19,262][INFO ][o.o.s.s.a.AsyncQueryExecutorServiceImpl] CreateAsyncQueryRequest: DROP MATERIALIZED VIEW myglue_test.default.count_by_status
[2024-09-02T15:24:19,307][INFO ][o.o.j.s.JobScheduler     ] Descheduling jobId: flint_myglue_test_default_count_by_status
[2024-09-02T15:24:19,348][INFO ][o.o.s.s.s.OpenSearchAsyncQueryScheduler]  Unscheduled job for jobId: flint_myglue_test_default_count_by_status

Vacuum should remove schedule

% curl -X POST -H "Content-Type: application/json" -d '{
  "datasource": "myglue_test",
  "lang": "sql",
  "query": "VACUUM MATERIALIZED VIEW myglue_test.default.count_by_status"
}' http://localhost:9200/_plugins/_async_query
{
  "queryId": "Zk05NXdZdFNET215Z2x1ZV90ZXN0"
}%

[2024-09-02T15:56:22,959][INFO ][o.o.s.s.a.AsyncQueryExecutorServiceImpl] CreateAsyncQueryRequest: VACUUM MATERIALIZED VIEW myglue_test.default.count_by_status
[2024-09-02T15:56:23,027][INFO ][o.o.s.s.f.o.FlintIndexOpVacuum] Vacuuming Flint index flint_myglue_test_default_count_by_status
[2024-09-02T15:56:23,095][INFO ][o.o.c.m.MetadataDeleteIndexService] [flint_myglue_test_default_count_by_status/50Au8FsdRPeLJxqLIIFnDA] deleting index                                    
[2024-09-02T15:56:23,151][INFO ][o.o.s.s.f.OpenSearchFlintIndexClient]  OpenSearch index delete result: true

green open .async-query-scheduler ooAqua8HSqyFGhfa1R_NUA 1 0 1 0 7.8kb 7.8kb
green open .async-query-scheduler ooAqua8HSqyFGhfa1R_NUA 1 0 0 1 11kb 11kb

Related Issues

#2833

Follow ups

The curreent RefreshQueryHandler doesn't check whether an index is in refreshing state before sending EMR job, with scheduler this can be optimized

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a 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.

Signed-off-by: Louis Chu <[email protected]>

ykmr1224 · 2024-09-03T18:00:54Z

async-query-core/src/main/java/org/opensearch/sql/spark/dispatcher/model/FlintIndexOptions.java

+  public boolean isExternalScheduler() {
+    // Default is false, which means using internal scheduler to refresh the index.


Internal/External sounds ambiguous to me. Can we rephrase or add documentation?

It's documented here https://github.com/opensearch-project/opensearch-spark/blob/main/docs/index.md#flint-index-refresh

Does internal means within spark? I think it is difficult to find that documentation from this option defined in SQL plugin. And I would imagine internal is internal to OpenSearch...

yeah internal means within spark and external could be anything.

I don't want to create a blocker now, but @ykmr1224 raised a good question.

We should have the name of this flag from the perspective of spark. Probably use_spark_scheduler = True | False would make more sense because we actually don't care about what an external scheduler is?

from spark perspective, we want to have extensibility among external Schedulers in case they have difference behavior

async-query-core/src/test/java/org/opensearch/sql/spark/asyncquery/AsyncQueryCoreIntegTest.java

...uery-core/src/test/java/org/opensearch/sql/spark/flint/operation/FlintIndexOpVacuumTest.java

async-query/src/main/java/org/opensearch/sql/spark/scheduler/parser/IntervalScheduleParser.java

async-query/src/main/java/org/opensearch/sql/spark/scheduler/job/ScheduledAsyncQueryJob.java

...ry/src/main/java/org/opensearch/sql/spark/scheduler/model/ScheduledAsyncQueryJobRequest.java

ykmr1224 · 2024-09-03T18:37:44Z

async-query/src/main/java/org/opensearch/sql/spark/scheduler/parser/IntervalScheduleParser.java

+          "^(\\d+)\\s*(years?|months?|weeks?|days?|hours?|minutes?|minute|mins?|seconds?|secs?|milliseconds?|millis?|micros?|nanoseconds?|nanos?)$",
+          Pattern.CASE_INSENSITIVE);
+
+  public static IntervalSchedule parse(String intervalStr, Instant startTime) {


Is it a right decision to implement it by ourselves? Is there common format for time duration and library for that?

Is it a right decision to implement it by ourselves? Is there common format for time duration and library for that?

The problem here is to converting Spark CalendarInterval to OpenSearch job scheduler interval.

Why do we use Spark CalendarInterval?

Spark CalendarInterval is the interface for spark scheduler, we will need this when convert stream job to external scheduler

...rc/test/java/org/opensearch/sql/spark/scheduler/model/ScheduledAsyncQueryJobRequestTest.java

Signed-off-by: Louis Chu <[email protected]>

async-query-core/src/main/antlr/FlintSparkSqlExtensions.g4

async-query-core/src/main/java/org/opensearch/sql/spark/scheduler/AsyncQueryScheduler.java

vamsimanohar

LGTM.
Are we tracking the optimization of using spark's scheduler in case of lower refresh intervals as a separate issue?

Signed-off-by: Louis Chu <[email protected]>

noCharger · 2024-09-04T16:46:55Z

LGTM. Are we tracking the optimization of using spark's scheduler in case of lower refresh intervals as a separate issue?

opensearch-project/opensearch-spark#622

penghuo · 2024-09-04T15:51:21Z

...query/src/main/java/org/opensearch/sql/spark/scheduler/job/ScheduledAsyncQueryJobRunner.java

+            LOGGER.error(throwable);
+          }
+        };
+    threadPool.generic().submit(runnable);


minor: Is it considered best practice to submit tasks to a generic thread pool across all job scheduler clients?

addressed in f9fe064

Signed-off-by: Louis Chu <[email protected]>

* Flint query scheduler part 2 Signed-off-by: Louis Chu <[email protected]> * spotless apply Signed-off-by: Louis Chu <[email protected]> * Add UT Signed-off-by: Louis Chu <[email protected]> * Resolve comments Signed-off-by: Louis Chu <[email protected]> * Add more UTs Signed-off-by: Louis Chu <[email protected]> * Resolve comments Signed-off-by: Louis Chu <[email protected]> * Use SQL thread pool Signed-off-by: Louis Chu <[email protected]> --------- Signed-off-by: Louis Chu <[email protected]> (cherry picked from commit 729bb13) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Flint query scheduler part 2 * spotless apply * Add UT * Resolve comments * Add more UTs * Resolve comments * Use SQL thread pool --------- (cherry picked from commit 729bb13) Signed-off-by: Louis Chu <[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>

seankao-az · 2024-09-05T02:08:24Z

Found some bug after merge..
in AsyncQueryCoreIntegTest.java
some verifyStoreJobMetadataCalled only use 1 argument, when 2 is expected
This is a change introduced in #2955

Edit: fixed in #2973

* Flint query scheduler part 2 * spotless apply * Add UT * Resolve comments * Add more UTs * Resolve comments * Use SQL thread pool --------- (cherry picked from commit 729bb13) Signed-off-by: Louis Chu <[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>

noCharger added 3 commits September 3, 2024 01:29

Flint query scheduler part 2

b09e99b

Signed-off-by: Louis Chu <[email protected]>

spotless apply

1741bd1

Signed-off-by: Louis Chu <[email protected]>

Add UT

04ea971

Signed-off-by: Louis Chu <[email protected]>

noCharger marked this pull request as ready for review September 3, 2024 16:55

noCharger requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsimanohar, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG, ykmr1224 and LantaoJin as code owners September 3, 2024 16:55

noCharger added the v2.17.0 label Sep 3, 2024

noCharger self-assigned this Sep 3, 2024

noCharger added the enhancement New feature or request label Sep 3, 2024

ykmr1224 reviewed Sep 3, 2024

View reviewed changes

noCharger added 2 commits September 3, 2024 14:54

Resolve comments

d2c316b

Signed-off-by: Louis Chu <[email protected]>

Add more UTs

83d3687

Signed-off-by: Louis Chu <[email protected]>

vamsimanohar reviewed Sep 4, 2024

View reviewed changes

async-query-core/src/main/antlr/FlintSparkSqlExtensions.g4 Show resolved Hide resolved

vamsimanohar reviewed Sep 4, 2024

View reviewed changes

async-query-core/src/main/java/org/opensearch/sql/spark/scheduler/AsyncQueryScheduler.java Show resolved Hide resolved

vamsimanohar reviewed Sep 4, 2024

View reviewed changes

async-query-core/src/main/java/org/opensearch/sql/spark/scheduler/AsyncQueryScheduler.java Show resolved Hide resolved

vamsimanohar previously approved these changes Sep 4, 2024

View reviewed changes

noCharger dismissed vamsimanohar’s stale review via c85ed20 September 4, 2024 16:22

Resolve comments

1e489b5

Signed-off-by: Louis Chu <[email protected]>

noCharger force-pushed the feature-query-scheduler-part-2 branch from c85ed20 to 1e489b5 Compare September 4, 2024 16:27

noCharger requested review from ykmr1224 and vamsimanohar September 4, 2024 17:02

penghuo reviewed Sep 4, 2024

View reviewed changes

ykmr1224 previously approved these changes Sep 4, 2024

View reviewed changes

noCharger dismissed ykmr1224’s stale review via c1f0054 September 4, 2024 19:03

Use SQL thread pool

f9fe064

Signed-off-by: Louis Chu <[email protected]>

noCharger force-pushed the feature-query-scheduler-part-2 branch from c1f0054 to f9fe064 Compare September 4, 2024 19:04

noCharger requested review from penghuo and ykmr1224 September 4, 2024 19:05

ykmr1224 approved these changes Sep 4, 2024

View reviewed changes

penghuo approved these changes Sep 4, 2024

View reviewed changes

seankao-az added backport 2.17 backport 2.x labels Sep 4, 2024

seankao-az merged commit 729bb13 into opensearch-project:main Sep 4, 2024
12 of 16 checks passed

opensearch-trigger-bot bot mentioned this pull request Sep 4, 2024

[Backport 2.x] Flint query scheduler part 2 #2974

Merged

opensearch-trigger-bot bot mentioned this pull request Sep 4, 2024

[Backport 2.17] Flint query scheduler part 2 #2975

Merged

seankao-az mentioned this pull request Sep 5, 2024

Fix handler for existing query #2968

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flint query scheduler part 2 #2961

Flint query scheduler part 2 #2961

noCharger commented Sep 3, 2024 •

edited

Loading

ykmr1224 Sep 3, 2024

noCharger Sep 3, 2024

ykmr1224 Sep 4, 2024

vamsimanohar Sep 4, 2024

vamsimanohar Sep 4, 2024

noCharger Sep 4, 2024

ykmr1224 Sep 3, 2024

noCharger Sep 3, 2024

ykmr1224 Sep 4, 2024

noCharger Sep 4, 2024

vamsimanohar left a comment

noCharger commented Sep 4, 2024

penghuo Sep 4, 2024

noCharger Sep 4, 2024

seankao-az commented Sep 5, 2024 •

edited

Loading

		public boolean isExternalScheduler() {
		// Default is false, which means using internal scheduler to refresh the index.

Flint query scheduler part 2 #2961

Flint query scheduler part 2 #2961

Conversation

noCharger commented Sep 3, 2024 • edited Loading

Description

Local sanity test

Drop should unschedule

Vacuum should remove schedule

Related Issues

Follow ups

Check List

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

vamsimanohar left a comment

Choose a reason for hiding this comment

noCharger commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seankao-az commented Sep 5, 2024 • edited Loading

noCharger commented Sep 3, 2024 •

edited

Loading

seankao-az commented Sep 5, 2024 •

edited

Loading