Remove direct ClusterState access in LocalClusterState #2696

fddattal · 2024-05-24T23:12:33Z

Description

This commit refactors the LocalClusterState singleton to instead access cluster state through the org.opensearch.client.Client abstract interface.

Issues Resolved

[RFC] Cloud Native SQL Plugin OpenSearch#13274

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-05-28T23:05:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.33%. Comparing base (3a28d2a) to head (bc189aa).
Report is 2 commits behind head on main.

❗ Current head bc189aa differs from pull request most recent head e67da5f

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2696      +/-   ##
============================================
- Coverage     95.33%   95.33%   -0.01%     
- Complexity     5089     5097       +8     
============================================
  Files           494      496       +2     
  Lines         14287    14297      +10     
  Branches        957      957              
============================================
+ Hits          13621    13630       +9     
- Misses          645      646       +1     
  Partials         21       21

Flag	Coverage Δ
sql-engine	`95.33% <ø> (-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.

andrross

Looks like you've got some spotless violations here:

FAILURE: Build completed with 2 failures.

1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':spotlessJavaCheck'.
> The following files had format violations:
      legacy/src/main/java/org/opensearch/sql/legacy/esdomain/LocalClusterState.java

andrross · 2024-05-29T18:11:12Z

plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java

@@ -202,6 +201,7 @@ public Collection<Object> createComponents(
    dataSourceService.createDataSource(defaultOpenSearchDataSourceMetadata());
    LocalClusterState.state().setClusterService(clusterService);
    LocalClusterState.state().setPluginSettings((OpenSearchSettings) pluginSettings);
+    LocalClusterState.state().setClient(client);


"LocalClusterState" is a misnomer now that it is just a pass-through to making a call via the Client, though renaming might make this change more intrusive.

andrross · 2024-05-29T18:12:37Z

legacy/src/main/java/org/opensearch/sql/legacy/esdomain/LocalClusterState.java

 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.index.IndexNotFoundException;
 import org.opensearch.sql.common.setting.Settings;
 import org.opensearch.sql.legacy.esdomain.mapping.IndexMappings;
 import org.opensearch.sql.opensearch.setting.OpenSearchSettings;

+import java.util.Arrays;


You should generally avoid re-ordering imports unless the new order follows a project-wide convention.

Sounds good! I'll submit a new revision with this change reverted

andrross · 2024-05-29T18:21:32Z

legacy/src/main/java/org/opensearch/sql/legacy/esdomain/LocalClusterState.java

+   * Sets the plugin's settings.
+   * @param settings The non-null plugin settings instance
+   */
+  public void setPluginSettings(@NonNull OpenSearchSettings settings) {


Slightly outside the scope of this PR, but it's not immediately obvious why this class is registering a settings update consumer and maintaining it's local cache of settings in latestSettings. It appears to be duplicating what is already happening in OpenSearchSettings. I wonder if getSettingValue() can be changed to just be a pass-through to OpenSearchSettings and all usage of the ClusterService can be removed from this class?

I get the feeling that you're right, but I also haven't spent enough time around it to be sure

saratvemulapalli · 2024-05-30T15:42:03Z

legacy/src/main/java/org/opensearch/sql/legacy/esdomain/LocalClusterState.java

-  public void setResolver(IndexNameExpressionResolver resolver) {
-    this.resolver = resolver;
-  }
-
  private LocalClusterState() {


No-op method, do we need this ?

I'm keeping it here to keep the constructor visibility rules the same. We could also use lombok to add an auto-generated private constructor

Update: I've decided to use lombok to generate the constructor instead

saratvemulapalli · 2024-05-30T16:01:32Z

legacy/src/main/java/org/opensearch/sql/legacy/esdomain/LocalClusterState.java

-      } else {
-        mappings = findMappings(state, concreteIndices, fieldFilter);
-      }
+      Map<String, MappingMetadata> mappingMetadata = client.admin()


trying to understand, we are removing SQL cached mappings and always fetch it from client.
Why do we always want to fetch it from client?

The mappings are already cached locally in the cluster state. By fetching via the client we are removing the data synchronization issues between cluster service and this class and avoiding the need for push based notifications to keep data in sync

saratvemulapalli · 2024-05-30T16:02:25Z

legacy/src/main/java/org/opensearch/sql/legacy/esdomain/LocalClusterState.java

+      Map<String, MappingMetadata> mappingMetadata = client.admin()
+              .indices()
+              .prepareGetMappings(indices)
+              .setLocal(true)


I presume this always gets mappings from local node, what if the data is stale?

If the data is stale, it will behave as it does today as even today's access is resolved locally.

That being said, it will fail requests with a 4xx as the fields are missing

We can accept this as it is the same as V2. Any thoughts on the long-term implications if schema freshness is critical?

…Mappings Signed-off-by: Frank Dattalo <[email protected]>

rupal-bq

LGTM Thanks for the changes.

penghuo

Thx!

penghuo · 2024-06-06T01:04:27Z

legacy/src/main/java/org/opensearch/sql/legacy/esdomain/LocalClusterState.java

+      Map<String, MappingMetadata> mappingMetadata = client.admin()
+              .indices()
+              .prepareGetMappings(indices)
+              .setLocal(true)


We can accept this as it is the same as V2. Any thoughts on the long-term implications if schema freshness is critical?

fddattal force-pushed the local_cluster_state_refactor_main branch 4 times, most recently from 4f21fe5 to bc189aa Compare May 25, 2024 00:46

fddattal marked this pull request as ready for review May 25, 2024 00:51

fddattal requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsi-amazon, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG and ykmr1224 as code owners May 25, 2024 00:51

fddattal mentioned this pull request May 28, 2024

[RFC] Cloud Native SQL Plugin opensearch-project/OpenSearch#13274

Closed

andrross reviewed May 29, 2024

View reviewed changes

saratvemulapalli reviewed May 30, 2024

View reviewed changes

Remove Direct ClusterState access in LocalClusterState for IndexField…

e67da5f

…Mappings Signed-off-by: Frank Dattalo <[email protected]>

fddattal force-pushed the local_cluster_state_refactor_main branch from bc189aa to e67da5f Compare May 30, 2024 19:38

rupal-bq approved these changes Jun 5, 2024

View reviewed changes

penghuo approved these changes Jun 6, 2024

View reviewed changes

penghuo merged commit 3f1e3bd into opensearch-project:main Jun 6, 2024
20 checks passed

penghuo added the backport 2.x label Jun 6, 2024

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

[Backport 2.x] Remove direct ClusterState access in LocalClusterState #2717

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove direct ClusterState access in LocalClusterState #2696

Remove direct ClusterState access in LocalClusterState #2696

fddattal commented May 24, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading

andrross left a comment

andrross May 29, 2024

fddattal May 30, 2024

andrross May 29, 2024

fddattal May 30, 2024

andrross May 29, 2024

fddattal May 30, 2024

saratvemulapalli May 30, 2024

fddattal May 30, 2024

fddattal May 30, 2024

saratvemulapalli May 30, 2024

fddattal May 30, 2024

saratvemulapalli May 30, 2024

fddattal May 30, 2024

penghuo Jun 6, 2024

rupal-bq left a comment

penghuo left a comment

penghuo Jun 6, 2024

Remove direct ClusterState access in LocalClusterState #2696

Remove direct ClusterState access in LocalClusterState #2696

Conversation

fddattal commented May 24, 2024 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

andrross 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

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

rupal-bq left a comment

Choose a reason for hiding this comment

penghuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fddattal commented May 24, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading