Add metadata transformations with metadata tuples #1148
Conversation
Signed-off-by: Andre Kurait <akurait@amazon.com>
AndreKurait
force-pushed
the
MetadataTransforms
branch
from
fbecea3
to
488c664
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1148 +/- ##
============================================
+ Coverage 80.78% 80.84% +0.06%
- Complexity 2947 2983 +36
============================================
Files 399 406 +7
Lines 15089 15215 +126
Branches 1017 1019 +2
============================================
+ Hits 12190 12301 +111
- Misses 2288 2289 +1
- Partials 611 625 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
AndreKurait
requested review from
chelma,
gregschohn,
lewijacn,
mikaylathompson,
peternied and
sumobrian
as code owners
RFS/src/main/java/org/opensearch/migrations/bulkload/worker/IndexRunner.java
Outdated
Show resolved
Hide resolved
MetadataMigration/src/main/java/org/opensearch/migrations/commands/Evaluate.java
Outdated
Show resolved
Hide resolved
|
|
||
| import lombok.extern.slf4j.Slf4j; | ||
|
|
||
| /** Shared functionality between migration and evaluation commands */ | ||
| @Slf4j | ||
| public abstract class MigratorEvaluatorBase { | ||
| public static final String NOOP_TRANSFORMATION_CONFIG = "[" + |
There was a problem hiding this comment.
Lets remove this, if there is no transformation config why create and use the factory?
...va/org/opensearch/migrations/bulkload/transformers/TransformerToIJsonTransformerAdapter.java
Show resolved
Hide resolved
| var transformedTemplates = Stream.concat(Stream.concat( | ||
| legacyTemplates.stream(), | ||
| indexTemplates.stream()), | ||
| componentTemplates.stream() | ||
| ) | ||
| .map(this::transformMigrationItem).collect(Collectors.toList()); |
There was a problem hiding this comment.
Why combine all the different migration item types into an undifferentiated stream, transform, then split them up again? Wouldn't it be easier and more efficient to just apply the transform to each class individually?
| log.atInfo().setMessage("BeforeJsonGlobal: {}").addArgument(() -> printMap(inputJson)).log(); | ||
| var afterJson = transformer.transformJson(inputJson); | ||
| log.atInfo().setMessage("AfterJsonGlobal: {}").addArgument(() -> printMap(afterJson)).log(); |
There was a problem hiding this comment.
It's interesting to have a pre-processor transform for the global metadata but not the index settings. Is this an opinionated stance or just a matter of current necessity? Also - how does this fit into the bigger picture we previously discussed of having user-supplied bulk pre/post processing transforms then line-item transforms for each individual MigrationItem between those two steps?
| @NoArgsConstructor(force = true) // For Jackson | ||
| @JsonAutoDetect(fieldVisibility = JsonAutoDetect.Visibility.NONE, getterVisibility = JsonAutoDetect.Visibility.NONE) | ||
| @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, property = "type") |
There was a problem hiding this comment.
Thanks for cleaning up all the Jackson marshalling for these data classes!
RFS/src/main/java/org/opensearch/migrations/bulkload/worker/IndexRunner.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andre Kurait <akurait@amazon.com>
There was a problem hiding this comment.
Thanks for the tests, looks like maybe there is some kind of refactoring we can do in the future to cleanup the setup / verification that is very similar between the tests, but this gives me a lot of confidence that we will know when we break something - much appreciated.
Seems like there is a one off system index that is causing the new test cases to fail and some style cop changes, but the bones of this good to me once CI is passing.
Description
[Describe what this change achieves]
Issues Resolved
[List any issues this PR will resolve]
Is this a backport? If so, please add backport PR # and/or commits #
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
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.