Support recovery for index with external scheduler #717

noCharger · 2024-09-30T06:59:02Z

Description

Support recovery for index with external scheduler
Support ALTER scheduler_mode between internal and external in single statement
Add index level metrics for refresh query, with dimension (clientId, dataSource, indexName)
Support external scheduler with index name including -
optimize build.sbt on dependency package complie

Issues Resolved

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.

dai-chen · 2024-10-01T22:18:46Z

flint-spark-integration/src/main/scala/org/apache/spark/sql/flint/config/FlintSparkConf.scala

-    EXTERNAL_SCHEDULER_INTERVAL_THRESHOLD.readFrom(reader)
+  def externalSchedulerIntervalThreshold(): String = {
+    val value = EXTERNAL_SCHEDULER_INTERVAL_THRESHOLD.readFrom(reader)
+    if (value.trim.isEmpty) FlintOptions.DEFAULT_EXTERNAL_SCHEDULER_INTERVAL else value


Is default value case already covered above by createWithDefault?

It might be a bug - conf can be empty string and createWithDefault will not trigger

if so, should we fix the bug?

override def readFrom(reader: ConfigReader): String = { readOptionKeyString(reader) .orElse(readFromConf()) .getOrElse(defaultValue) }

If the spark configuration value is an empty string, readOptionKeyString returns Some("") as expected, while Option.getOrElse returns an empty string. Change the behavior to override empty string with default value means that the entire empty string use case is disallowed.

...ion/src/main/scala/org/opensearch/flint/spark/scheduler/FlintSparkJobSchedulingService.scala

dai-chen · 2024-10-02T00:03:57Z

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSpark.scala

-            FlintSparkIndexRefresh
-              .create(indexName, index.get)
-              .start(spark, flintSparkConf)
+            flintIndexMetadataService.updateIndexMetadata(indexName, updatedIndex.metadata())


This is only for external scheduler?

For recover index, we need to persist the schduler_mode field for backward compatibility on both internal and external cases.

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSpark.scala

Signed-off-by: Louis Chu <clingzhi@amazon.com>

build.sbt

flint-core/src/main/scala/org/opensearch/flint/core/FlintOptions.java

penghuo · 2024-10-10T00:03:20Z

flint-spark-integration/src/main/scala/org/apache/spark/sql/flint/config/FlintSparkConf.scala

-    EXTERNAL_SCHEDULER_INTERVAL_THRESHOLD.readFrom(reader)
+  def externalSchedulerIntervalThreshold(): String = {
+    val value = EXTERNAL_SCHEDULER_INTERVAL_THRESHOLD.readFrom(reader)
+    if (value.trim.isEmpty) FlintOptions.DEFAULT_EXTERNAL_SCHEDULER_INTERVAL else value


if so, should we fix the bug?

penghuo · 2024-10-10T03:09:24Z

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSpark.scala

+        FlintSparkIndexRefresh
+          .create(updatedIndex.name(), updatedIndex)
+          .validate(spark)


original logic is guarded by tanslog, is it still required? @dai-chen

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSpark.scala

penghuo · 2024-10-10T03:13:09Z

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSpark.scala

+    }.keySet
+
+    if (changedOptions.isEmpty) {
+      throw new IllegalArgumentException("No options updated")


Previous exception message "auto_refresh option must be updated" is more understandable.

It's no longer the case since this PR supports single Alter to switch scheduler_mode, while auto_refresh option should not change

penghuo · 2024-10-10T03:22:50Z

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSpark.scala

      throw new IllegalArgumentException(
-        s"Altering index to ${refreshMode} refresh only allows options: ${allowedOptionNames}")
+        s"$context only allows changing: $allowedOptions. Invalid options: $invalidOptions")


L477-L509 is refactor logic?

Why we change error message?

Yes the whole validateUpdateAllowed is refactored to support switch scheduler_mode in single alter, while auto_refresh option should not change

is there a test for this case?

added more ITs around validation logic 03d4c47

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSpark.scala

penghuo · 2024-10-10T03:30:19Z

...park-integration/src/test/scala/org/opensearch/flint/spark/FlintSparkIndexBuilderSuite.scala

+        Some("10 minutes"),
+        None),
+      (
+        "throw exception when interval below threshold but mode is external",


not releate to this PR, How does user configure External Scheduler? Is it any doc mention it?

yes mentioned here https://github.com/opensearch-project/opensearch-spark/blob/main/docs/index.md#configurations

I only find the external-scheduler definiton, but there is no guidance how does user configure it.
could u create a issue to cover it?

The configuration is here https://github.com/opensearch-project/opensearch-spark/blob/main/docs/index.md#configurations

spark.flint.job.externalScheduler.enabled: default is false. enable external scheduler for flint auto refresh to schedule refresh job outside of spark. spark.flint.job.externalScheduler.interval: default is 5 minutes. a string of refresh interval for external scheduler to trigger index refresh.

do you mean we need a seperate section to explain this in detail?

my questions is, if user set spark.flint.job.externalScheduler.enabled = true, does user need additinal configuration to run externalScheduler?

...-integration/src/main/scala/org/opensearch/flint/spark/refresh/IncrementalIndexRefresh.scala

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSparkIndexOptions.scala

flint-spark-integration/src/main/scala/org/opensearch/flint/spark/FlintSparkIndexFactory.scala

Signed-off-by: Louis Chu <clingzhi@amazon.com>

penghuo

Thx!

* Support recovery for index with external scheduler Signed-off-by: Louis Chu <clingzhi@amazon.com> * Improve default option update logic Signed-off-by: Louis Chu <clingzhi@amazon.com> * Resolve comments Signed-off-by: Louis Chu <clingzhi@amazon.com> * Add index metrics Signed-off-by: Louis Chu <clingzhi@amazon.com> * Remove debugging log and refactor updateSchedulerMode Signed-off-by: Louis Chu <clingzhi@amazon.com> * refactor metrics with aop Signed-off-by: Louis Chu <clingzhi@amazon.com> * Add more IT Signed-off-by: Louis Chu <clingzhi@amazon.com> --------- Signed-off-by: Louis Chu <clingzhi@amazon.com> (cherry picked from commit a345373) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Support recovery for index with external scheduler * Improve default option update logic * Resolve comments * Add index metrics * Remove debugging log and refactor updateSchedulerMode * refactor metrics with aop * Add more IT --------- (cherry picked from commit a345373) Signed-off-by: Louis Chu <clingzhi@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>

…t#717) * Support recovery for index with external scheduler Signed-off-by: Louis Chu <clingzhi@amazon.com> * Improve default option update logic Signed-off-by: Louis Chu <clingzhi@amazon.com> * Resolve comments Signed-off-by: Louis Chu <clingzhi@amazon.com> * Add index metrics Signed-off-by: Louis Chu <clingzhi@amazon.com> * Remove debugging log and refactor updateSchedulerMode Signed-off-by: Louis Chu <clingzhi@amazon.com> * refactor metrics with aop Signed-off-by: Louis Chu <clingzhi@amazon.com> * Add more IT Signed-off-by: Louis Chu <clingzhi@amazon.com> --------- Signed-off-by: Louis Chu <clingzhi@amazon.com>

dai-chen reviewed Oct 2, 2024

View reviewed changes

Support recovery for index with external scheduler

01d76a8

Signed-off-by: Louis Chu <clingzhi@amazon.com>

noCharger force-pushed the recover-external-scheduler branch 2 times, most recently from fdbf5e7 to 3487f11 Compare October 8, 2024 08:25

Improve default option update logic

4a104cb

Signed-off-by: Louis Chu <clingzhi@amazon.com>

noCharger force-pushed the recover-external-scheduler branch from 3487f11 to 4a104cb Compare October 8, 2024 10:26

noCharger marked this pull request as ready for review October 8, 2024 10:26

noCharger requested review from rupal-bq, vamsi-amazon, penghuo, seankao-az, anirudha, kaituo and YANG-DB as code owners October 8, 2024 10:26

noCharger self-assigned this Oct 8, 2024

noCharger added 0.5 backport 0.5 backport 0.5-nexus labels Oct 8, 2024

noCharger added 2 commits October 8, 2024 07:44

Resolve comments

5d8ae39

Signed-off-by: Louis Chu <clingzhi@amazon.com>

Add index metrics

88bd413

Signed-off-by: Louis Chu <clingzhi@amazon.com>

noCharger requested a review from LantaoJin as a code owner October 9, 2024 23:35

penghuo reviewed Oct 10, 2024

View reviewed changes

dai-chen reviewed Oct 10, 2024

View reviewed changes

noCharger added 2 commits October 10, 2024 15:26

Remove debugging log and refactor updateSchedulerMode

c141662

Signed-off-by: Louis Chu <clingzhi@amazon.com>

refactor metrics with aop

6622931

Signed-off-by: Louis Chu <clingzhi@amazon.com>

noCharger requested review from penghuo and dai-chen October 11, 2024 01:42

noCharger force-pushed the recover-external-scheduler branch from 143c847 to 03d4c47 Compare October 11, 2024 23:43

Add more IT

e59a1f8

Signed-off-by: Louis Chu <clingzhi@amazon.com>

noCharger force-pushed the recover-external-scheduler branch from 03d4c47 to e59a1f8 Compare October 11, 2024 23:57

penghuo approved these changes Oct 12, 2024

View reviewed changes

dai-chen approved these changes Oct 12, 2024

View reviewed changes

noCharger merged commit a345373 into opensearch-project:main Oct 14, 2024
4 checks passed

opensearch-trigger-bot bot mentioned this pull request Oct 14, 2024

[Backport 0.5] Support recovery for index with external scheduler #776

Merged

opensearch-trigger-bot bot mentioned this pull request Oct 14, 2024

[Backport 0.5-nexus] Support recovery for index with external scheduler #777

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support recovery for index with external scheduler #717

Support recovery for index with external scheduler #717

noCharger commented Sep 30, 2024 •

edited

Loading

dai-chen Oct 1, 2024

noCharger Oct 4, 2024

penghuo Oct 10, 2024

noCharger Oct 10, 2024

dai-chen Oct 2, 2024

noCharger Oct 3, 2024

penghuo Oct 10, 2024

penghuo Oct 10, 2024

penghuo Oct 10, 2024

noCharger Oct 10, 2024

penghuo Oct 10, 2024

noCharger Oct 10, 2024

penghuo Oct 11, 2024

noCharger Oct 11, 2024

penghuo Oct 10, 2024

noCharger Oct 10, 2024

penghuo Oct 11, 2024

noCharger Oct 11, 2024

penghuo Oct 12, 2024

penghuo left a comment

Support recovery for index with external scheduler #717

Support recovery for index with external scheduler #717

Conversation

noCharger commented Sep 30, 2024 • edited Loading

Description

Issues Resolved

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

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

penghuo left a comment

Choose a reason for hiding this comment

noCharger commented Sep 30, 2024 •

edited

Loading