Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scheduler_mode index option #415

Merged
merged 6 commits into from
Jul 15, 2024

Conversation

noCharger
Copy link
Collaborator

@noCharger noCharger commented Jul 10, 2024

Description

Add scheduler_mode index option

Issues Resolved

#417

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.

@noCharger noCharger force-pushed the Add-scheduler-mode branch 3 times, most recently from f2c4a4a to 9f6a0de Compare July 10, 2024 18:15
@noCharger noCharger marked this pull request as ready for review July 10, 2024 18:40
@noCharger noCharger force-pushed the Add-scheduler-mode branch 4 times, most recently from abebc9e to 4358e83 Compare July 11, 2024 00:29
@@ -43,9 +44,14 @@ class AutoIndexRefresh(indexName: String, index: FlintSparkIndex)
!isTableProviderSupported(spark, index),
"Index auto refresh doesn't support Hive table")

// Checkpoint location is required if mandatory option set
// Checkpoint location is required if mandatory option set or external scheduler is used
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is External Auto Refresh == Incremental Refresh? I thought we can just use IncrementalIndexRefresh and decide this in factory method in FlintSparkIndexRefresh?

Copy link
Collaborator Author

@noCharger noCharger Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is External Auto Refresh == Incremental Refresh?

Physically, yes. But, logically, should we consider it the same as IncrementalIndexRefresh?

I thought we can just use IncrementalIndexRefresh and decide this in factory method in FlintSparkIndexRefresh?

In the current implementation, the index option remains the source of truth for every operation, therefore it is fine. If RefreshMode is used directly to assign index options in the future, it may result in a mismatch.

One example, if the checkpoint is missing, the output would be

"requirement failed: Checkpoint location is required by incremental refresh"

instead of

"requirement failed: Checkpoint location is required for external scheduler"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, the confusion is refreshMode in FlintSparkIndexRefresh makes use of the same enum as the code manipulates index option. This actually should be separated enum which describes the refresh execution mode instead. We can open an refactoring issue for tracking. Thanks!

Copy link
Collaborator Author

@noCharger noCharger Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment resolved ed23902
And follow-up tracking #431

docs/index.md Show resolved Hide resolved
Signed-off-by: Louis Chu <[email protected]>
Signed-off-by: Louis Chu <[email protected]>
Signed-off-by: Louis Chu <[email protected]>
Signed-off-by: Louis Chu <[email protected]>
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

@noCharger noCharger merged commit 890930c into opensearch-project:main Jul 15, 2024
4 checks passed
@dai-chen dai-chen added enhancement New feature or request 0.5 labels Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.5 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants