[Remote Routing Table]Read remote index routing #13894
Conversation
Arpit-Bandejiya
changed the title
| }).collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public static List<String> getIndicesRoutingDeleted(RoutingTable previousRoutingTable, RoutingTable currentRoutingTable) { |
There was a problem hiding this comment.
This function is used for getting the deleted indices for creating diff in manifest
| return deletedIndicesRouting; | ||
| } | ||
|
|
||
| public static List<String> getIndicesRoutingUpdated(RoutingTable previousRoutingTable, RoutingTable currentRoutingTable) { |
There was a problem hiding this comment.
This function is used for getting the updated index routing for creating diff in manifest.
| return null; | ||
| } | ||
|
|
||
| public List<ClusterMetadataManifest.UploadedIndexMetadata> getUpdatedIndexRoutingTableMetadata(List<String> updatedIndicesRouting, List<ClusterMetadataManifest.UploadedIndexMetadata> allIndicesRouting) { |
There was a problem hiding this comment.
Used to create the Metadata from the list of updated indicesRouting from Manifest in read flow.
|
❌ 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? |
| } catch (IOException e) { | ||
| logger.info("RoutingTable read failed with error: {}", e.toString()); | ||
| } |
There was a problem hiding this comment.
use the correct exception logging
There was a problem hiding this comment.
Thowed RemoteStateTransfterException.
| } catch (IOException e) { | ||
| logger.info("RoutingTable read failed with error: {}", e.toString()); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Lets throw exception in case of failures, returning null can generate NPEs in the request flow.
| 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; |
There was a problem hiding this comment.
Can we reuse DiffableUtils rather than replicating the logic here
| private String getIndexRoutingFileName() { | ||
| return String.join( | ||
| DELIMITER, | ||
| INDEX_ROUTING_FILE_PREFIX, | ||
| RemoteStoreUtils.invertLong(System.currentTimeMillis()) | ||
| ); | ||
|
|
||
| } |
There was a problem hiding this comment.
cc: @ashking94 for reviewing prefix changes
There was a problem hiding this comment.
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
| return () -> readAsync( | ||
| blobContainer, | ||
| blobFileName, | ||
| threadPool.executor(ThreadPool.Names.GENERIC), | ||
| ActionListener.wrap(response -> latchedActionListener.onResponse(new RemoteIndexRoutingResult(index.getName(), response.readIndexRoutingTable(index))), latchedActionListener::onFailure) | ||
| ); |
There was a problem hiding this comment.
Lets use a dedicated thread pool for remote state interactions
There was a problem hiding this comment.
Where can I find the reference for RemoteIndexRoutingResult ?
| * | ||
| * @opensearch.internal | ||
| */ | ||
| public class RemoteRoutingTableService implements Closeable { |
There was a problem hiding this comment.
Would prefer AbstractLifecycleComponent to be implemented
There was a problem hiding this comment.
Started an conversation on the parent PR: https://github.com/opensearch-project/OpenSearch/pull/13304/files#r1623843757
There was a problem hiding this comment.
This is taken care in #13577
|
|
||
| } | ||
|
|
||
| public CheckedRunnable<IOException> getAsyncIndexMetadataReadAction( |
There was a problem hiding this comment.
Where can I see the consumer of this function?
| blobStoreRepository = (BlobStoreRepository) repository; | ||
| } | ||
|
|
||
| public static class RemoteIndexRoutingResult { |
There was a problem hiding this comment.
Why is this class needed? IndexRoutingTable already holds Index info
There was a problem hiding this comment.
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.
| 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())) { |
There was a problem hiding this comment.
Don't we need to compare versions?
There was a problem hiding this comment.
Do you mean when the previsionRoutingTable of older version of manifest and we are on the latest one?
|
❌ 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? |
|
❌ 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>
Arpit-Bandejiya
force-pushed
the
read-remote-routing
branch
from
129b1d3
to
768585e
Compare
|
❌ 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? |
| boolean readTemplatesMetadata, | ||
| boolean readDiscoveryNodes, | ||
| boolean readClusterBlocks, | ||
| List<UploadedIndexMetadata> indicesRoutingToRead, |
There was a problem hiding this comment.
How are we getting this list indicesRoutingToRead
| } | ||
| } | ||
|
|
||
| public List<ClusterMetadataManifest.UploadedIndexMetadata> getUpdatedIndexRoutingTableMetadata(List<String> updatedIndicesRouting, List<ClusterMetadataManifest.UploadedIndexMetadata> allIndicesRouting) { |
There was a problem hiding this comment.
Where is this getting called from in RemoteClusterStateService?
There was a problem hiding this comment.
Added in the flow now.
| ClusterState.Builder clusterStateBuilder = ClusterState.builder(previousState); | ||
| Map<String, IndexRoutingTable> indicesRouting = new HashMap<>(previousState.routingTable().getIndicesRouting()); | ||
|
|
||
|
|
||
| readIndexRoutingTableResults.forEach(indexRoutingTable -> { | ||
| indicesRouting.put(indexRoutingTable.getIndex().getName(), indexRoutingTable); | ||
| }); |
There was a problem hiding this comment.
What if there are index in previousState which are now deleted?
There was a problem hiding this comment.
Covered in the main call, added the flow in the PR.
| ); | ||
|
|
||
| indicesRoutingToDelete.forEach(allUploadedIndicesRouting::remove); | ||
|
|
There was a problem hiding this comment.
nit: avoid unnecessary line breaks
| 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); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Can we use interfaces from #13785
| Map<String, UploadedMetadataAttribute> customToRead, | ||
| boolean readCoordinationMetadata, | ||
| boolean readSettingsMetadata, | ||
| boolean readTransientSettingsMetadata, | ||
| boolean readTemplatesMetadata, | ||
| boolean readDiscoveryNodes, | ||
| boolean readClusterBlocks, |
There was a problem hiding this comment.
Create an inner class instead?
Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
|
❌ 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? |
|
Since changes are merged in #13924. Closing this PR. |
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
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.