Skip to content

Alias index transform #1049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

n-dohrmann
Copy link
Contributor

Issue #, if available:
#656

Description of changes:
Adding to createTargetIndex method in the TransformIndexer module - want to support using aliases for variable target indices on transform jobs.

CheckList:

  • [ x] 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.

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 62.64%. Comparing base (5aeed27) to head (7cf342a).
Report is 86 commits behind head on main.

Files with missing lines Patch % Lines
...arch/indexmanagement/transform/TransformIndexer.kt 0.00% 9 Missing ⚠️

❌ Your project check has failed because the head coverage (62.64%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (5aeed27) and HEAD (7cf342a). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (5aeed27) HEAD (7cf342a)
4 2
Additional details and impacted files
@@              Coverage Diff              @@
##               main    #1049       +/-   ##
=============================================
- Coverage     74.81%   62.64%   -12.18%     
+ Complexity     2810     2339      -471     
=============================================
  Files           367      367               
  Lines         16518    16527        +9     
  Branches       2362     2366        +4     
=============================================
- Hits          12358    10353     -2005     
- Misses         2857     5020     +2163     
+ Partials       1303     1154      -149     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -63,6 +65,20 @@ class TransformIndexer(
throw TransformIndexException("Failed to create the target index")
}
}
val writeIndexMetadata = clusterService.state().metadata.indicesLookup[targetIndex]!!.writeIndex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should inside the below if block?

if (clusterService.state().metadata.hasAlias(targetIndex)) {
// return error if no write index with the alias
if (writeIndexMetadata == null) {
throw TransformIndexException("Target index alias has no write index")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

target_index [$targetIndex] is an alias but doesn't have write index

Comment on lines 75 to 81
val putMappingReq = PutMappingRequest(writeIndexMetadata?.index?.name).source(targetFieldMappings)
val mapResp: AcknowledgedResponse = client.admin().indices().suspendUntil {
putMapping(putMappingReq)
}
if (!mapResp.isAcknowledged) {
logger.error("Target index mapping request failed")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also move this inside to above if block?

@HenryTheSir
Copy link

Hi,
is there any news on this feature?
Best regards

@MahendraAkkina
Copy link

MahendraAkkina commented Mar 29, 2025

Any update on this? @bowenlan-amzn @n-dohrmann

@bowenlan-amzn
Copy link
Member

@HenryTheSir @MahendraAkkina I don't think @n-dohrmann still works on this. Would you want to create another PR from this?

@MahendraAkkina
Copy link

@bowenlan-amzn Thanks for checking on this. I am not familiar with the dev process. Is there a doc talking about it?. I see that you were involved heavily in reviewing it. So wanted to check if its something you can help with creating a new PR and help with getting it merged?
cc: @HenryTheSir

@bowenlan-amzn
Copy link
Member

@MahendraAkkina this doc should help https://github.com/opensearch-project/index-management/blob/main/DEVELOPER_GUIDE.md
I'm mainly working on another projects now and limiting here, sorry about that.
Let me tag current active maintainer to see if he can help @vaibhoag

@MahendraAkkina
Copy link

@bowenlan-amzn @vaibhoag I am looking at the fix and am little confused. I see that createTargetIndex is failing as it is not able to create an index which is actually an alias. The code fix has additional code after the first if. I think all this code needs to go under first if check. @bowenlan-amzn Is that what you were suggesting?

@MahendraAkkina
Copy link

Something like this, that is I moved the additional code into the first if

    private suspend fun createTargetIndex(targetIndex: String, targetFieldMappings: Map<String, Any>) {
        if (!clusterService.state().routingTable.hasIndex(targetIndex)) {
            if (clusterService.state().metadata.hasAlias(targetIndex)) {
                // return error if no write index with the alias
                val writeIndexMetadata = clusterService.state().metadata.indicesLookup[targetIndex]!!.writeIndex
                if (writeIndexMetadata == null) {
                    throw TransformIndexException("target_index [$targetIndex] is an alias but doesn't have write index")
                }
                val putMappingReq = PutMappingRequest(writeIndexMetadata.index?.name).source(targetFieldMappings)
                val mapResp: AcknowledgedResponse = client.admin().indices().suspendUntil {
                    putMapping(putMappingReq)
                }
                if (!mapResp.isAcknowledged) {
                    logger.error("Target index mapping request failed")
                }
            } else {
                val transformTargetIndexMapping = TargetIndexMappingService.createTargetIndexMapping(targetFieldMappings)
                val request = CreateIndexRequest(targetIndex).mapping(transformTargetIndexMapping)
                // TODO: Read in the actual mappings from the source index and use that
                val response: CreateIndexResponse = client.admin().indices().suspendUntil { create(request, it) }
                if (!response.isAcknowledged) {
                    logger.error("Failed to create the target index $targetIndex")
                    throw TransformIndexException("Failed to create the target index")
                }
            }
        }
    }

@bowenlan-amzn
Copy link
Member

@MahendraAkkina commented on Apr 16, 2025, 6:00 PM PDT:

I am looking at the fix and am little confused. I see that createTargetIndex is failing as it is not able to create an index which is actually an alias. The code fix has additional code after the first if. I think all this code needs to go under first if check. Is that what you were suggesting?

Yeah, we should move the alias check to the top, first see if provided targetIndex is an alias, then see if we want to create that index.

@MahendraAkkina
Copy link

Instead of updating the mappings every time a transform runs, especially in continuous mode, I think choosing not to update them for aliases. My reasoning is that updating mappings on each run feels inconsistent with how index mappings are handled, where they are typically defined once at index creation. In the specific scenario with rollover aliases, we should rely on index templates to pre-define all the necessary fields when new indices are created, thus eliminating the need for continuous mapping updates.

@vaibhoag, thoughts? Can you or someone else at amazon :-) help with contributing to the this project?
cc: @bowenlan-amzn

    private suspend fun createTargetIndex(targetIndex: String, targetFieldMappings: Map<String, Any>) {
        if (!clusterService.state().routingTable.hasIndex(targetIndex)) {
            if (clusterService.state().metadata.hasAlias(targetIndex)) {
                logger.debug("Checking to see if target_index [$targetIndex] that is an alias has write index")
                // return error if no write index with the alias
                val writeIndexMetadata = clusterService.state().metadata.indicesLookup[targetIndex]!!.writeIndex
                if (writeIndexMetadata == null) {
                    throw TransformIndexException("target_index [$targetIndex] is an alias but doesn't have write index")
                }
            } else {
                val transformTargetIndexMapping = TargetIndexMappingService.createTargetIndexMapping(targetFieldMappings)
                val request = CreateIndexRequest(targetIndex).mapping(transformTargetIndexMapping)
                // TODO: Read in the actual mappings from the source index and use that
                val response: CreateIndexResponse = client.admin().indices().suspendUntil { create(request, it) }
                if (!response.isAcknowledged) {
                    logger.error("Failed to create the target index $targetIndex")
                    throw TransformIndexException("Failed to create the target index")
                }
            }
        }
    }

@vaibhoag
Copy link
Contributor

Hey @MahendraAkkina I think instead of updating mappings, we should first validate the writeIndex mapping with targetFieldMappings and if that is false, then we should update the mappings

Actually this depends on how we are creating targetAlias, are we expecting users to create it? @bowenlan-amzn

@MahendraAkkina
Copy link

MahendraAkkina commented Apr 30, 2025

@vaibhoag That should work. I would assume it would check this every time the transform operation runs (Example: In the case when continuous is set to true and is scheduled to run every hour, this check would happen once an hour). We can imagine the writeIndex to be the same for a long time (as it typically depends on the rollover policy). Might not be a big performance hit, but just wanted to point out.
Alternatively, I am ok with having a limitation for alias case where we expect the mappings to already exist in the writeIndex. Rationale being in the case of regular index, the current code is trying to add mappings only in the case index does not exist, so since we are working with existing writeIndex, we can take this approach.

@MahendraAkkina
Copy link

@vaibhoag Will you be able to contribute by creating a new PR and help with getting it merged? Would be very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants