[Metadata Immutability] Change different indices lookup objects from array type to lists by akolarkunnu · Pull Request #14557 · opensearch-project/OpenSearch

akolarkunnu · 2024-06-26T12:28:51Z

Description

Changed the arrays to immutable List instances, added new versions of the getters which returns List instances.

Related Issues

Resolves #8647

Signed-off-by: Abdul Muneer Kolarkunnu muneer.kolarkunnu@netapp.com

Check List

New functionality includes testing.
All tests pass
~~New functionality has been documented.~~
New functionality has javadoc added
~~API changes companion pull request created.~~
~~Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)~~
Commits are signed per the DCO using --signoff
~~Commit changes are listed out in CHANGELOG.md file (See: Changelog)~~
~~Public documentation issue/PR created~~

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.

github-actions · 2024-06-26T12:52:06Z

❌ Gradle check result for 34849c8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-06-26T13:15:03Z

❌ Gradle check result for 5affe72: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

akolarkunnu · 2024-06-26T14:52:36Z

Can anyone help me to rerun the test suites which are failed ?

CHANGELOG-3.0.md

andrross · 2024-06-26T17:59:18Z

Can anyone help me to rerun the test suites which are failed ?

@akolarkunnu Can you rebase this PR with the latest from main? The recent release incremented version numbers and as a result backward compatibility tests are now failing here.

github-actions · 2024-06-27T06:27:58Z

✅ Gradle check result for 779e864: SUCCESS

codecov · 2024-06-27T06:29:03Z

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.71%. Comparing base (0684342) to head (5c0949b).
Report is 14 commits behind head on main.

Files	Patch %	Lines
.../cluster/metadata/IndexNameExpressionResolver.java	75.00%	1 Missing and 1 partial ⚠️
...n/cluster/health/TransportClusterHealthAction.java	0.00%	1 Missing ⚠️
...arch/index/recovery/RemoteStoreRestoreService.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #14557      +/-   ##
============================================
+ Coverage     71.68%   71.71%   +0.03%     
- Complexity    62365    62388      +23     
============================================
  Files          5148     5148              
  Lines        293297   293279      -18     
  Branches      42384    42383       -1     
============================================
+ Hits         210239   210325      +86     
+ Misses        65717    65601     -116     
- Partials      17341    17353      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrross

Looks good to me. @sandeshkr419 can you review this as well?

sandeshkr419 · 2024-06-29T05:52:24Z

server/src/main/java/org/opensearch/cluster/metadata/Metadata.java

+     * Returns all the concrete indices that are not hidden in the format of array of strings.
     */
    public String[] getConcreteVisibleIndices() {
+        return visibleIndices.toArray(String[]::new);


I don't think we have any usage left for the existing methods which are returning as arrays.

We probably just need a single method for each case which returns it as a list:

public List<String> getConcreteVisibleIndices() { return visibleIndices; }

This comment applies to all utilities in the scope of this PR.

I had asked this query in bug - #8647 (comment)

Reply form Andrew was "This class is exposed to all OpenSearch plugins." Based on this I introduced new APIs.

Got it. Makes sense.

I'm wondering what should be the plan to mark the old APIs as deprecated so that we remove them entirely in a later release.

@andrross any suggestions? How about we have different set of changes for 3.0 vs 2.x - in 3.0 we change the API construct - breaking changes, in 2.x we just add extra APIs.

Yeah, it's fair game to put the @Deprecated annotation on these APIs and then remove them on the main branch for the 3.0 release.

Updated this PR for 3.0 release as suggested. Just changed the return type of existing APIs from String array to Collection types.

Thanks @akolarkunnu Changes make sense for 3.0 now with a minor code comment. Rest of it LGTM!

Also, removed 'backport-2.x' label to avoid auto-backporting to 2.x - I assume we will do a separate set of manual changes altogether for 2.x

sandeshkr419 · 2024-06-29T05:57:38Z

CHANGELOG.md

 - [Writable Warm] Add composite directory implementation and integrate it with FileCache ([12782](https://github.com/opensearch-project/OpenSearch/pull/12782))
 - Fix race condition while parsing derived fields from search definition ([14445](https://github.com/opensearch-project/OpenSearch/pull/14445))
 - Add allowlist setting for ingest-common processors ([#14439](https://github.com/opensearch-project/OpenSearch/issues/14439))
+- Add new APIs in the Metadata to get different indices in the form of List ([#14557](https://github.com/opensearch-project/OpenSearch/pull/14557))


I think we need to just modify the utilities and not add any new ones. So we can edit the CHANGELOG to be something like "Modify the utilities in the Metadata.....". What do you say?

Updated CHANGELOG-3.0.md file as suggested, since this change is only for 3.0 release and fro 2.x it will be different changes.

Thanks @akolarkunnu Changes make sense for 3.0 now with a minor code comment. Rest of it LGTM!

github-actions · 2024-07-09T11:06:17Z

❌ Gradle check result for 1cfda8d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-07-09T11:31:18Z

❌ Gradle check result for 88cd3df: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2024-07-09T11:59:35Z

✅ Gradle check result for 098be2e: SUCCESS

github-actions · 2024-07-09T12:03:59Z

❕ Gradle check result for 5c0949b: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

akolarkunnu · 2024-07-09T13:12:44Z

Known flaky org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests.testTaskResourceTrackingDuringTaskCancellation #14293
Known flaky org.opensearch.repositories.azure.AzureBlobContainerRetriesTests.testWriteBlobWithRetries #14318
Known flaky org.opensearch.index.remote.RemoteSegmentTransferTrackerTests.testComputeTimeLagOnUpdate #14325

sandeshkr419 · 2024-07-10T19:51:14Z

...r/src/main/java/org/opensearch/action/admin/cluster/health/TransportClusterHealthAction.java

        if (request.level().equals(ClusterHealthRequest.Level.AWARENESS_ATTRIBUTES)) {
            String awarenessAttribute = request.getAwarenessAttribute();
-            concreteIndices = clusterState.getMetadata().getConcreteAllIndices();
+            concreteIndices = clusterState.getMetadata().getConcreteAllIndices().toArray(String[]::new);


Does it make sense to change concreteIndices to lists as well? Instead of doing a back and forth to arrays?

I mean atleast in OpenSearch packages, we can ensure that we consume these concreteIndices as lists directly at all places. I mean, it looks like concreteIndices is actually just used in the constructor so, it might make sense to use it as list itself.

Addressed it in new PR #14723 (This PR has screwed-up after multiple rebase)

sandeshkr419 · 2024-07-10T19:51:28Z

server/src/main/java/org/opensearch/cluster/health/ClusterStateHealth.java

     */
    public ClusterStateHealth(final ClusterState clusterState) {
-        this(clusterState, clusterState.metadata().getConcreteAllIndices());
+        this(clusterState, clusterState.metadata().getConcreteAllIndices().toArray(String[]::new));


Same comment as above- Does it make sense to change concreteIndices to lists as well? Instead of doing a back and forth to arrays?

Addressed it in new PR #14723 (This PR has screwed-up after multiple rebase)

github-actions · 2024-07-11T09:25:34Z

❌ Gradle check result for 26886e0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

akolarkunnu · 2024-07-11T10:18:58Z

This PR has screwed-up after multiple rebase, so created a new PR - #14723

akolarkunnu requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, dblock, dbwiddis, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners June 26, 2024 12:28

github-actions bot added Cluster Manager enhancement Enhancement or improvement to existing feature or request good first issue Good for newcomers labels Jun 26, 2024

andrross reviewed Jun 26, 2024

View reviewed changes

CHANGELOG-3.0.md Outdated Show resolved Hide resolved

akolarkunnu requested review from jed326 and peternied as code owners June 27, 2024 05:09

akolarkunnu force-pushed the metadata branch from 7d9aa58 to 779e864 Compare June 27, 2024 05:36

opensearch-ci-bot mentioned this pull request Jun 27, 2024

[AUTOCUT] Gradle Check Flaky Test Report for ICacheKeySerializerTests #14504

Closed

andrross added the backport 2.x Backport to 2.x branch label Jun 27, 2024

andrross approved these changes Jun 27, 2024

View reviewed changes

andrross requested a review from sandeshkr419 June 27, 2024 20:10

sandeshkr419 assigned sandeshkr419 and akolarkunnu Jun 29, 2024

sandeshkr419 reviewed Jun 29, 2024

View reviewed changes

akolarkunnu requested a review from ashking94 as a code owner July 9, 2024 10:57

akolarkunnu force-pushed the metadata branch from 1cfda8d to 098be2e Compare July 9, 2024 11:05

sandeshkr419 reviewed Jul 10, 2024

View reviewed changes

sandeshkr419 added >breaking Identifies a breaking change. and removed backport 2.x Backport to 2.x branch labels Jul 10, 2024

akolarkunnu force-pushed the metadata branch from 5c0949b to 26886e0 Compare July 11, 2024 09:18

akolarkunnu closed this Jul 11, 2024

akolarkunnu force-pushed the metadata branch from 26886e0 to 2d8c68c Compare July 11, 2024 09:22

sandeshkr419 mentioned this pull request Jul 21, 2024

[Metadata Immutability] Change different indices lookup objects from array type to lists #14723

Open

9 tasks

This was referenced Sep 9, 2024

[AUTOCUT] Gradle Check Flaky Test Report for ResourceAwareTasksTests #14293

Open

[AUTOCUT] Gradle Check Flaky Test Report for RemoteSegmentTransferTrackerTests #14325

Closed

[AUTOCUT] Gradle Check Flaky Test Report for AzureBlobContainerRetriesTests #14318

Open

Conversation

akolarkunnu commented Jun 26, 2024

Description

Related Issues

Check List

Uh oh!

github-actions bot commented Jun 26, 2024

Uh oh!

github-actions bot commented Jun 26, 2024

Uh oh!

akolarkunnu commented Jun 26, 2024

Uh oh!

Uh oh!

andrross commented Jun 26, 2024

Uh oh!

github-actions bot commented Jun 27, 2024

Uh oh!

codecov bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andrross left a comment

Choose a reason for hiding this comment

Uh oh!

sandeshkr419 Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandeshkr419 Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 9, 2024

Uh oh!

github-actions bot commented Jul 9, 2024

Uh oh!

github-actions bot commented Jul 9, 2024

Uh oh!

github-actions bot commented Jul 9, 2024

Uh oh!

akolarkunnu commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandeshkr419 Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 11, 2024

Uh oh!

akolarkunnu commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

codecov bot commented Jun 27, 2024 •

edited

Loading

sandeshkr419 Jun 29, 2024 •

edited

Loading

sandeshkr419 Jun 30, 2024 •

edited

Loading

akolarkunnu commented Jul 9, 2024 •

edited

Loading

sandeshkr419 Jul 10, 2024 •

edited

Loading