Bug Fix , delete OpenSearch index when DROP INDEX #2250

penghuo · 2023-10-06T22:51:29Z

Description

bug fix drop index

Issues Resolved

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.

Signed-off-by: Peng Huo <[email protected]>

vamsi-amazon · 2023-10-06T23:02:44Z

spark/src/main/java/org/opensearch/sql/spark/flint/FlintIndexMetadataReader.java

@@ -11,5 +11,5 @@ public interface FlintIndexMetadataReader {
   * @param indexDetails indexDetails.
   * @return jobId.
   */
-  String getJobIdFromFlintIndexMetadata(IndexDetails indexDetails);
+  FlintIndexMetadata getJobIdFromFlintIndexMetadata(IndexDetails indexDetails);


Nit: change method name.
Also, return type in comments.

vamsi-amazon · 2023-10-06T23:07:16Z

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

+        JSONObject dummyData = new JSONObject();
+        dummyData.put("result", new JSONArray());
+        dummyData.put("schema", new JSONArray());
+        dummyData.put("applicationId", "fakeDropId");


nit: change to fakeApplicationId.

vamsi-amazon · 2023-10-06T23:09:14Z

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

+      String queryId =
+          RandomStringUtils.randomAlphanumeric(PREFIX_LEN)
+              + String.join(DELIMITER, status, errorMsg);
+      return Base64.getEncoder().encodeToString(queryId.getBytes(StandardCharsets.UTF_8));


What is the usual length of this result?
If it is more or less similar to emrJobId it would be good.

SXlRbFhVczI1bEZBSUxFRA== similar

vamsi-amazon · 2023-10-06T23:09:33Z

spark/src/main/java/org/opensearch/sql/spark/dispatcher/model/IndexDetails.java

@@ -23,4 +23,38 @@ public class IndexDetails {
  private Boolean autoRefresh = false;
  private boolean isDropIndex;
  private FlintIndexType indexType;
+
+  public String openSearchIndexName() {


Makes sense 👍

codecov · 2023-10-06T23:09:42Z

Codecov Report

Merging #2250 (f325997) into main (55e8e84) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #2250   +/-   ##
=========================================
  Coverage     96.59%   96.60%           
- Complexity     4730     4736    +6     
=========================================
  Files           439      441    +2     
  Lines         12691    12728   +37     
  Branches        871      875    +4     
=========================================
+ Hits          12259    12296   +37     
  Misses          424      424           
  Partials          8        8

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

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

Files	Coverage Δ
...OpensearchAsyncQueryJobMetadataStorageService.java	`100.00% <100.00%> (ø)`
...rch/sql/spark/dispatcher/SparkQueryDispatcher.java	`100.00% <100.00%> (ø)`
...earch/sql/spark/dispatcher/model/IndexDetails.java	`100.00% <100.00%> (ø)`
...opensearch/sql/spark/flint/FlintIndexMetadata.java	`100.00% <100.00%> (ø)`
.../sql/spark/flint/FlintIndexMetadataReaderImpl.java	`100.00% <100.00%> (ø)`

vamsi-amazon · 2023-10-06T23:12:19Z

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

@@ -53,6 +67,8 @@ public class SparkQueryDispatcher {

  private FlintIndexMetadataReader flintIndexMetadataReader;

+  private Client client;


[Not a hard requirement if possible]
Instead of having client in SparkQueryDispatcher: can we move all the Flint Index logic to One class.

For example: Change FlintIndexMetadataReader -> FlintIndexHanlder and handle deletion logic in that class.

agree.
do it later. fix bug first.

vamsi-amazon · 2023-10-06T23:14:41Z

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

+      try {
+        AcknowledgedResponse response =
+            client.admin().indices().delete(new DeleteIndexRequest().indices(indexName)).get();
+        if (!response.isAcknowledged()) {
+          errorMsg = String.format("failed to delete index %s", indexName);
+          LOG.error(errorMsg);
+        }


As suggested in above comment can we move this to FlintIndexHandler or some other class.

agree.
do it later. fix bug first.

vamsi-amazon · 2023-10-06T23:15:25Z

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

-    String dropIndexDummyJobId = RandomStringUtils.randomAlphanumeric(16);
+    FlintIndexMetadata indexMetadata =
+        flintIndexMetadataReader.getJobIdFromFlintIndexMetadata(indexDetails);
+    // if index is created without auto refresh. there is no job to cancel.


Thanks for fixing this. I missed a lot of cases.

vamsi-amazon · 2023-10-06T23:17:00Z

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

@@ -64,6 +80,11 @@ public DispatchQueryResponse dispatch(DispatchQueryRequest dispatchQueryRequest)
  }

  public JSONObject getQueryResponse(AsyncQueryJobMetadata asyncQueryJobMetadata) {
+    // todo. refactor query process logic in plugin.


What does this mean?

using if ... else.. not a good solution, we need to add abstraction. e.g. QueryTracker.

dai-chen · 2023-10-06T23:19:24Z

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

+    try {
+      if (indexMetadata.isAutoRefresh()) {
+        emrServerlessClient.cancelJobRun(
+            dispatchQueryRequest.getApplicationId(), indexMetadata.getJobId());


so if this AppId is different from what's in metadata, we either fail here or assume job is already migrated to this App ?

for open source, there is only one appid.
we should handle multiple appId cases in future.

Signed-off-by: Peng Huo <[email protected]>

* Bug Fix, delete opensearch index when DROP INDEX Signed-off-by: Peng Huo <[email protected]> * address comments Signed-off-by: Peng Huo <[email protected]> --------- Signed-off-by: Peng Huo <[email protected]> (cherry picked from commit 0c78105) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Bug Fix, delete opensearch index when DROP INDEX * address comments --------- (cherry picked from commit 0c78105) Signed-off-by: Peng Huo <[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>

penghuo added 2 commits October 6, 2023 15:46

Bug Fix, delete opensearch index when DROP INDEX

32a8759

Signed-off-by: Peng Huo <[email protected]>

merge from main

3111ecd

Signed-off-by: Peng Huo <[email protected]>

penghuo requested review from pjfitzgibbons, ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsi-amazon, Swiddis, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto and GumpacG as code owners October 6, 2023 22:51

penghuo added backport 2.11 backport 2.x labels Oct 6, 2023

vamsi-amazon reviewed Oct 6, 2023

View reviewed changes

dai-chen reviewed Oct 6, 2023

View reviewed changes

address comments

f325997

Signed-off-by: Peng Huo <[email protected]>

dai-chen approved these changes Oct 6, 2023

View reviewed changes

vamsi-amazon approved these changes Oct 6, 2023

View reviewed changes

penghuo merged commit 0c78105 into opensearch-project:main Oct 6, 2023
21 checks passed

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

[Backport 2.x] Bug Fix , delete OpenSearch index when DROP INDEX #2251

Merged

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

[Backport 2.11] Bug Fix , delete OpenSearch index when DROP INDEX #2252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Fix , delete OpenSearch index when DROP INDEX #2250

Bug Fix , delete OpenSearch index when DROP INDEX #2250

penghuo commented Oct 6, 2023 •

edited

Loading

vamsi-amazon Oct 6, 2023

penghuo Oct 6, 2023

vamsi-amazon Oct 6, 2023

penghuo Oct 6, 2023

vamsi-amazon Oct 6, 2023 •

edited

Loading

penghuo Oct 6, 2023

vamsi-amazon Oct 6, 2023

codecov bot commented Oct 6, 2023 •

edited

Loading

vamsi-amazon Oct 6, 2023

penghuo Oct 6, 2023

vamsi-amazon Oct 6, 2023

penghuo Oct 6, 2023

vamsi-amazon Oct 6, 2023

vamsi-amazon Oct 6, 2023

penghuo Oct 6, 2023

dai-chen Oct 6, 2023 •

edited

Loading

penghuo Oct 6, 2023

		@@ -53,6 +67,8 @@ public class SparkQueryDispatcher {

		private FlintIndexMetadataReader flintIndexMetadataReader;

		private Client client;

Bug Fix , delete OpenSearch index when DROP INDEX #2250

Bug Fix , delete OpenSearch index when DROP INDEX #2250

Conversation

penghuo commented Oct 6, 2023 • edited Loading

Description

Issues Resolved

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

vamsi-amazon Oct 6, 2023 • edited Loading

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 6, 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

dai-chen Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penghuo commented Oct 6, 2023 •

edited

Loading

vamsi-amazon Oct 6, 2023 •

edited

Loading

codecov bot commented Oct 6, 2023 •

edited

Loading

dai-chen Oct 6, 2023 •

edited

Loading