[Remote Routing Table]Read remote index routing by alchemist51 · Pull Request #13894 · opensearch-project/OpenSearch

alchemist51 · 2024-05-30T13:44:03Z

Description

This is an draft PR for remote index routing read. The PR is dependent on cluster state service publication PR #13835 and initial remote routing setup: #13304

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

alchemist51 · 2024-05-30T13:45:49Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        }).collect(Collectors.toList());
+    }
+
+    public static List<String> getIndicesRoutingDeleted(RoutingTable previousRoutingTable, RoutingTable currentRoutingTable) {


This function is used for getting the deleted indices for creating diff in manifest

alchemist51 · 2024-05-30T13:47:39Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        return deletedIndicesRouting;
+    }
+
+    public static List<String> getIndicesRoutingUpdated(RoutingTable previousRoutingTable, RoutingTable currentRoutingTable) {


This function is used for getting the updated index routing for creating diff in manifest.

alchemist51 · 2024-05-30T13:49:08Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        return null;
+    }
+
+    public List<ClusterMetadataManifest.UploadedIndexMetadata> getUpdatedIndexRoutingTableMetadata(List<String> updatedIndicesRouting, List<ClusterMetadataManifest.UploadedIndexMetadata> allIndicesRouting) {


Used to create the Metadata from the list of updated indicesRouting from Manifest in read flow.

github-actions · 2024-05-30T13:50:08Z

❌ Gradle check result for 933933c: 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?

himshikha · 2024-05-31T09:23:52Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        } catch (IOException e) {
+            logger.info("RoutingTable read failed with error: {}", e.toString());
+        }


throw exception here

use the correct exception logging

Thowed RemoteStateTransfterException.

himshikha · 2024-05-31T09:32:30Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        } catch (IOException e) {
+            logger.info("RoutingTable read failed with error: {}", e.toString());
+        }
+        return null;


Lets throw exception in case of failures, returning null can generate NPEs in the request flow.

himshikha · 2024-05-31T09:39:41Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+    public static List<String> getIndicesRoutingUpdated(RoutingTable previousRoutingTable, RoutingTable currentRoutingTable) {
+        List<String> updatedIndicesRouting = new ArrayList<>();
+        for(IndexRoutingTable currentIndicesRouting: currentRoutingTable.getIndicesRouting().values()) {
+            if(!previousRoutingTable.getIndicesRouting().containsKey(currentIndicesRouting.getIndex().getName())) {
+                // Latest Routing Table does not have entry for the index which means the index is created
+                updatedIndicesRouting.add(currentIndicesRouting.getIndex().getName());
+            } else {
+                if(previousRoutingTable.getIndicesRouting().get(currentIndicesRouting.getIndex().getName()).equals(currentIndicesRouting)) {
+                    // if the latest routing table has the same routing table as the previous routing table, then the index is not updated
+                    continue;
+                }
+                updatedIndicesRouting.add(currentIndicesRouting.getIndex().getName());
+            }
+        }
+        return updatedIndicesRouting;


Can we reuse DiffableUtils rather than replicating the logic here

Bukhtawar · 2024-06-02T16:36:14Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+    private String getIndexRoutingFileName() {
+        return String.join(
+            DELIMITER,
+            INDEX_ROUTING_FILE_PREFIX,
+            RemoteStoreUtils.invertLong(System.currentTimeMillis())
+        );
+
+    }


cc: @ashking94 for reviewing prefix changes

This function is not in use for read flow, got added by mistake. I think the following PR has changes related to the prefix hashing: https://github.com/opensearch-project/OpenSearch/pull/13870/files#diff-54b9eb2094b2f28930e8c235f57f00d8cb6547e790684cf54e533756e1251f85R216

Bukhtawar · 2024-06-02T16:40:22Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        return () -> readAsync(
+            blobContainer,
+            blobFileName,
+            threadPool.executor(ThreadPool.Names.GENERIC),
+            ActionListener.wrap(response -> latchedActionListener.onResponse(new RemoteIndexRoutingResult(index.getName(), response.readIndexRoutingTable(index))), latchedActionListener::onFailure)
+        );


Lets use a dedicated thread pool for remote state interactions

Where can I find the reference for RemoteIndexRoutingResult ?

Bukhtawar · 2024-06-02T16:41:19Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+ *
+ * @opensearch.internal
+ */
+public class RemoteRoutingTableService implements Closeable {


Would prefer AbstractLifecycleComponent to be implemented

Started an conversation on the parent PR: https://github.com/opensearch-project/OpenSearch/pull/13304/files#r1623843757

This is taken care in #13577

Bukhtawar · 2024-06-02T17:24:08Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+
+    }
+
+    public CheckedRunnable<IOException> getAsyncIndexMetadataReadAction(


Where can I see the consumer of this function?

https://github.com/shiv0408/OpenSearch/blob/bb335f6da175657a9ef082e3a5f44c9ee3c0d13b/server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java#L1024

Bukhtawar · 2024-06-02T18:15:58Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        blobStoreRepository = (BlobStoreRepository) repository;
+    }
+
+    public static class RemoteIndexRoutingResult {


Why is this class needed? IndexRoutingTable already holds Index info

The idea was to keep something similar to RemoteResult type of object. For now, I have removed it. We can take an call when the parent PRs are merged.

Bukhtawar · 2024-06-02T18:17:34Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+    public static List<String> getIndicesRoutingUpdated(RoutingTable previousRoutingTable, RoutingTable currentRoutingTable) {
+        List<String> updatedIndicesRouting = new ArrayList<>();
+        for(IndexRoutingTable currentIndicesRouting: currentRoutingTable.getIndicesRouting().values()) {
+            if(!previousRoutingTable.getIndicesRouting().containsKey(currentIndicesRouting.getIndex().getName())) {


Don't we need to compare versions?

Do you mean when the previsionRoutingTable of older version of manifest and we are on the latest one?

github-actions · 2024-06-03T11:13:50Z

❌ Gradle check result for a7fb428: 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-04T12:04:43Z

❌ Gradle check result for 129b1d3: 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?

Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>

github-actions · 2024-06-06T16:19:47Z

❌ Gradle check result for 768585e: 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?

himshikha · 2024-06-06T17:29:45Z

server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java

+        boolean readTemplatesMetadata,
+        boolean readDiscoveryNodes,
+        boolean readClusterBlocks,
+        List<UploadedIndexMetadata> indicesRoutingToRead,


How are we getting this list indicesRoutingToRead

himshikha · 2024-06-06T17:38:11Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        }
+    }
+
+    public List<ClusterMetadataManifest.UploadedIndexMetadata> getUpdatedIndexRoutingTableMetadata(List<String> updatedIndicesRouting, List<ClusterMetadataManifest.UploadedIndexMetadata> allIndicesRouting) {


Where is this getting called from in RemoteClusterStateService?

Added in the flow now.

himshikha · 2024-06-06T17:39:24Z

server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java

+        ClusterState.Builder clusterStateBuilder = ClusterState.builder(previousState);
+        Map<String, IndexRoutingTable> indicesRouting = new HashMap<>(previousState.routingTable().getIndicesRouting());
+
+
+        readIndexRoutingTableResults.forEach(indexRoutingTable -> {
+            indicesRouting.put(indexRoutingTable.getIndex().getName(), indexRoutingTable);
+        });


What if there are index in previousState which are now deleted?

Covered in the main call, added the flow in the PR.

Bukhtawar · 2024-06-06T17:49:17Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        );
+
+        indicesRoutingToDelete.forEach(allUploadedIndicesRouting::remove);
+


nit: avoid unnecessary line breaks

Bukhtawar · 2024-06-06T17:57:14Z

server/src/main/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableService.java

+        Index index,
+        LatchedActionListener<IndexRoutingTable> latchedActionListener) {
+        int idx = uploadedFilename.lastIndexOf("/");
+        String blobFileName = uploadedFilename.substring(idx+1);
+        BlobContainer blobContainer = blobStoreRepository.blobStore().blobContainer( BlobPath.cleanPath().add(uploadedFilename.substring(0,idx)));
+
+        return () -> readAsync(
+            blobContainer,
+            blobFileName,
+            index,
+            threadPool.executor(ThreadPool.Names.GENERIC),
+            ActionListener.wrap(response -> latchedActionListener.onResponse(response.getIndexRoutingTable()), latchedActionListener::onFailure)
+        );
+    }
+
+    public void readAsync(BlobContainer blobContainer, String name, Index index, ExecutorService executorService, ActionListener<RemoteIndexRoutingTable> listener) throws IOException {
+        executorService.execute(() -> {
+            try {
+                listener.onResponse(read(blobContainer, name, index));
+            } catch (Exception e) {
+                listener.onFailure(e);
+            }
+        });
+    }


Can we use interfaces from #13785

Bukhtawar · 2024-06-06T17:58:21Z

server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java

+        Map<String, UploadedMetadataAttribute> customToRead,
+        boolean readCoordinationMetadata,
+        boolean readSettingsMetadata,
+        boolean readTransientSettingsMetadata,
+        boolean readTemplatesMetadata,
+        boolean readDiscoveryNodes,
+        boolean readClusterBlocks,


Create an inner class instead?

Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>

github-actions · 2024-06-07T07:37:18Z

❌ Gradle check result for d254d16: null

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?

alchemist51 · 2024-06-10T04:45:30Z

Since changes are merged in #13924. Closing this PR.

alchemist51 changed the title ~~Read remote index routing~~ [Remote Routing Table]Read remote index routing May 30, 2024

alchemist51 commented May 30, 2024

View reviewed changes

himshikha reviewed May 31, 2024

View reviewed changes

Bukhtawar reviewed Jun 2, 2024

View reviewed changes

Add read flow for remote routing

768585e

Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>

alchemist51 force-pushed the read-remote-routing branch from 129b1d3 to 768585e Compare June 6, 2024 16:13

himshikha reviewed Jun 6, 2024

View reviewed changes

Bukhtawar reviewed Jun 6, 2024

View reviewed changes

Add more cluster service change for review

d254d16

Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>

alchemist51 closed this Jun 10, 2024


		}

		public CheckedRunnable<IOException> getAsyncIndexMetadataReadAction(

		);

		indicesRoutingToDelete.forEach(allUploadedIndicesRouting::remove);

Conversation

alchemist51 commented May 30, 2024

Description

Related Issues

Check List

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 May 30, 2024

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!

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!

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 Jun 3, 2024

Uh oh!

github-actions bot commented Jun 4, 2024

Uh oh!

github-actions bot commented Jun 6, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 7, 2024

Uh oh!