feat(schemaField): populate schemaFields with side effects #10928
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent update introduces the creation of a new configuration class for generating schema fields from schema metadata, modifications to batch processing, and side effects handling for schema fields. Significant refactoring is done to existing methods and classes to enhance their functionality, including the merging logic for maps and aspect filtering capabilities. Additionally, several test classes are added to ensure the robustness of these new implementations and changes. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
product
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (5)
metadata-service/services/src/main/java/com/linkedin/metadata/entity/restoreindices/RestoreIndicesArgs.java (1)
25-25: Addition ofaspectNamesfieldThe addition of the
aspectNamesfield initialized withCollections.emptyList()is a safe choice as it avoids mutable state issues. However, if there's a scenario whereaspectNamesneeds to be modified, consider using a mutable list initialized to an empty immutable list to avoidUnsupportedOperationException.- public List<String> aspectNames = Collections.emptyList(); + public List<String> aspectNames = new ArrayList<>();metadata-service/factories/src/main/java/com/linkedin/gms/factory/context/SystemOperationContextFactory.java (2)
43-45: Update method signatures to include new parameterThe invocations of
javaSystemOperationContextin bothSystemOperationContextFactory.javaandSystemUpdateConfig.javahave not been updated to include the newBaseElasticSearchComponentsparameter. This needs to be corrected to ensure proper dependency injection.
- Update the method signature in
SystemOperationContextFactory.javato include theBaseElasticSearchComponentsparameter.- Update the method signature in
SystemUpdateConfig.javato include theBaseElasticSearchComponentsparameter.Analysis chain
Parameter addition review in
javaSystemOperationContextThe addition of the
BaseElasticSearchComponentsparameter with@Qualifierannotation is a good practice for clarifying dependency injection requirements. This change likely improves the modularity and configurability of the operation context creation. Ensure that all callers of this method are updated to pass the required component.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure that all callers of `javaSystemOperationContext` are updated. rg --type java -A 5 "javaSystemOperationContext"Length of output: 2007
Script:
#!/bin/bash # Check for the updated invocation of javaSystemOperationContext in SystemOperationContextFactory.java rg --type java -A 5 "javaSystemOperationContext" metadata-service/factories/src/main/java/com/linkedin/gms/factory/context/SystemOperationContextFactory.java # Check for the updated invocation of javaSystemOperationContext in SystemUpdateConfig.java rg --type java -A 5 "javaSystemOperationContext" datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SystemUpdateConfig.javaLength of output: 1058
91-93: Potential Configuration Update IssueThe addition of the
BaseElasticSearchComponentsparameter inrestliSystemOperationContextappears to be isolated to theSystemOperationContextFactory.javafile. There are no corresponding updates in configuration files or service initializations.
- File:
metadata-service/factories/src/main/java/com/linkedin/gms/factory/context/SystemOperationContextFactory.java- Lines: 91-93
Please ensure that all necessary configuration files and service initializations are updated to reflect this change.
Analysis chain
Parameter addition review in
restliSystemOperationContextSimilar to the previous method, the addition of the
BaseElasticSearchComponentsparameter enhances the flexibility in configuration. It's important to verify that the configuration files and service initializations are updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for updates in configuration files related to `restliSystemOperationContext`. rg --type properties "restliSystemOperationContext"Length of output: 86
Script:
#!/bin/bash # Search for updates in configuration files related to `restliSystemOperationContext` specifically within .properties files. rg "restliSystemOperationContext" --glob "*.properties"Length of output: 55
Script:
#!/bin/bash # Search for references to `restliSystemOperationContext` across all file types to ensure comprehensive coverage. rg "restliSystemOperationContext"Length of output: 201
metadata-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.java (2)
343-396: Improve error handling infetchRequiredAspectsmethod.The
fetchRequiredAspectsmethod lacks explicit error handling mechanisms for scenarios where data might be missing or if there are issues during aspect retrieval. Consider adding try-catch blocks, logging, or conditional checks to handle these cases gracefully.
- File:
metadata-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.java- Lines: 343-396
Analysis chain
Review aspect fetching logic in
fetchRequiredAspects.This utility method fetches necessary aspects from the batch items, ensuring that all required data is available for processing. The method is well-implemented with clear separation of concerns and effective use of Java streams. However, ensure that the error handling for missing data is adequate.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling for missing data in fetchRequiredAspects method. # Test: Search for error handling patterns in this method. Expect: Adequate error handling for missing data scenarios. rg --type java -A 5 $'fetchRequiredAspects'Length of output: 2646
Script:
#!/bin/bash # Description: Verify error handling for missing data in fetchRequiredAspects method. # Test: Search for error handling patterns in fetchRequiredAspects method. rg --type java -A 50 'private static Map<Urn, Map<String, Aspect>> fetchRequiredAspects' metadata-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.javaLength of output: 2206
289-319: Review deletion MCP generation logic inbuildSchemaMetadataSchemaFieldDeleteMCPs.The
buildSchemaMetadataSchemaFieldDeleteMCPsmethod lacks test coverage for edge cases such as empty fields or missing aspect specs. It is recommended to add specific tests to ensure these scenarios are handled correctly.
- Add tests to cover edge cases for the
buildSchemaMetadataSchemaFieldDeleteMCPsmethod.Analysis chain
Review deletion MCP generation logic in
buildSchemaMetadataSchemaFieldDeleteMCPs.This method handles the generation of delete MCPs for schema fields when a dataset's schema metadata is deleted. The use of streams and mappings is efficient, but ensure that all edge cases, such as empty fields or missing aspect specs, are handled correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify handling of edge cases in deletion MCP generation. # Test: Search for test cases covering edge cases in this method. Expect: Comprehensive test coverage. rg --type java -A 5 $'buildSchemaMetadataSchemaFieldDeleteMCPs'Length of output: 1801
Script:
#!/bin/bash # Description: Search for test cases covering edge cases in the buildSchemaMetadataSchemaFieldDeleteMCPs method. # Find all Java test files that reference the buildSchemaMetadataSchemaFieldDeleteMCPs method. rg --type java 'buildSchemaMetadataSchemaFieldDeleteMCPs' -l | grep -i 'test'Length of output: 75
Script:
#!/bin/bash # Description: Search for any references to the buildSchemaMetadataSchemaFieldDeleteMCPs method across all Java files. # Find all Java files that reference the buildSchemaMetadataSchemaFieldDeleteMCPs method. rg --type java 'buildSchemaMetadataSchemaFieldDeleteMCPs' -lLength of output: 160
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SystemUpdateConfig.java (3 hunks)
- datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/system/schemafield/GenerateSchemaFieldsFromSchemaMetadata.java (1 hunks)
- datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/system/schemafield/GenerateSchemaFieldsFromSchemaMetadataStep.java (1 hunks)
- datahub-upgrade/src/test/java/com/linkedin/datahub/upgrade/schemafield/GenerateSchemaFieldsFromSchemaMetadataStepTest.java (1 hunks)
- entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/AspectsBatch.java (2 hunks)
- li-utils/src/main/java/com/linkedin/metadata/Constants.java (1 hunks)
- metadata-io/src/main/java/com/linkedin/metadata/entity/ebean/EbeanAspectDao.java (1 hunks)
- metadata-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.java (1 hunks)
- metadata-io/src/test/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffectTest.java (1 hunks)
- metadata-models/src/main/resources/entity-registry.yml (2 hunks)
- metadata-service/factories/src/main/java/com/linkedin/gms/factory/context/SystemOperationContextFactory.java (4 hunks)
- metadata-service/services/src/main/java/com/linkedin/metadata/entity/restoreindices/RestoreIndicesArgs.java (2 hunks)
Additional comments not posted (25)
datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/system/schemafield/GenerateSchemaFieldsFromSchemaMetadata.java (2)
13-17: Class documentation and logging setupThe class documentation is clear and concise, explaining the purpose of the class. The use of the
@Slf4jannotation for logging is appropriate here.
39-47: Method implementation review forsteps()The
steps()method correctly returns the_stepslist initialized in the constructor. This method is straightforward and adheres to the expected interface contract.datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SystemUpdateConfig.java (2)
138-178: Review of thejavaSystemOperationContextmethod.The method
javaSystemOperationContextis well-structured and uses dependency injection to configure the system operation context. It integrates various services and configurations, which is crucial for the system's operation. The use of@Primaryand@Nonnullannotations enhances the method's robustness by ensuring that the beans are correctly prioritized and that null values are handled properly.However, ensure that all services and components used in this method are properly initialized and available in the application context to prevent runtime issues.
Line range hint
3-31: Review of new imports and configurations.The imports added are extensive and seem to cover a wide range of functionalities including authentication, search, and entity services. Ensure that these imports are necessary and used within the file to avoid unused imports which can clutter the codebase.
metadata-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.java (7)
1-8: Ensure proper package and import organization.The package declaration and imports are appropriately organized and follow Java conventions. The static imports for constants are used effectively to improve code readability.
44-47: Class annotations and logging setup.The use of
@Slf4j,@Getter,@Setter, and@Accessors(chain = true)annotations are appropriate for this class, providing logging capabilities and simplifying the getter and setter boilerplate. This setup enhances code readability and maintainability.
51-52: Validate the set of required aspects.The static initialization of
REQUIRED_ASPECTSwithSCHEMA_METADATA_ASPECT_NAMEandSTATUS_ASPECT_NAMEis crucial for the operations of this class. This ensures that the necessary data is always fetched before processing, which is a good practice to prevent runtime errors due to missing data.
54-98: Review side effect application logic inapplyMCPSideEffect.This method orchestrates the side effect processing by filtering and mapping changes to appropriate handlers. The use of Java streams for filtering and flatMapping is efficient and clean. However, ensure that the complex logic within the flatMap operations is well-tested to handle all edge cases.
Verification successful
The
applyMCPSideEffectmethod is well-tested.The test cases in
SchemaFieldSideEffectTest.javacomprehensively cover the conditional paths and edge cases for theapplyMCPSideEffectmethod, ensuring the filtering and flatMapping logic is well-verified.
schemaMetadataToSchemaFieldKeyTeststatusToSchemaFieldStatusTestScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for applyMCPSideEffect method. # Test: Search for the test cases covering this method. Expect: At least one test case per conditional path. rg --type java -A 5 $'applyMCPSideEffect'Length of output: 10658
Script:
#!/bin/bash # Extract the test cases from SchemaFieldSideEffectTest.java to verify the coverage of applyMCPSideEffect method. # Find the file path of SchemaFieldSideEffectTest.java test_file_path=$(fd SchemaFieldSideEffectTest.java) # Extract the test cases from the identified test file rg --type java -A 10 'applyMCPSideEffect' $test_file_pathLength of output: 1387
Script:
#!/bin/bash # Extract the complete test methods from SchemaFieldSideEffectTest.java to verify the coverage of applyMCPSideEffect method. # Find the file path of SchemaFieldSideEffectTest.java test_file_path=$(fd SchemaFieldSideEffectTest.java) # Extract the complete test methods from the identified test file rg --type java -A 50 'public void' $test_file_pathLength of output: 9993
398-402: Validate utility methodgetSchemaFieldUrn.This method constructs a URN for schema fields based on a parent URN and a field. The implementation is straightforward and uses well-defined constants. Ensure that this method is covered by unit tests to verify the correctness of the URN format.
Verification successful
The utility method
getSchemaFieldUrnis covered by unit tests.The URN format generated by
getSchemaFieldUrnis indirectly validated in the test cases through thePlatformEventMatcher, which compares theentityUrnfield of theEntityChangeEventobjects.
EntityChangeEventGeneratorHookTest.javausesPlatformEventMatcherto verify the URN format.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for getSchemaFieldUrn method. # Test: Search for the test cases covering this method. Expect: At least one test case verifying URN format. rg --type java -A 5 $'getSchemaFieldUrn'Length of output: 27087
Script:
#!/bin/bash # Description: Verify that the test cases using getSchemaFieldUrn are checking the URN format. # Test: Search for assertions in the test files that call getSchemaFieldUrn. rg --type java -A 10 'getSchemaFieldUrn' | grep -E 'assert|verify'Length of output: 1351
Script:
#!/bin/bash # Description: Verify that the verifyProducePlatformEvent method checks the URN format. # Step 1: Find the implementation of verifyProducePlatformEvent. rg --type java 'void verifyProducePlatformEvent' -A 10Length of output: 2634
Script:
#!/bin/bash # Description: Verify that the PlatformEventMatcher checks the URN format. # Step 1: Find the implementation of PlatformEventMatcher. rg --type java 'class PlatformEventMatcher' -A 20Length of output: 3053
Script:
#!/bin/bash # Description: Verify that the payloadMatches method in PlatformEventMatcher checks the URN format. # Step 1: Find the complete implementation of payloadMatches method. rg --type java 'public boolean payloadMatches' -A 20Length of output: 3660
100-158: Review post-processing of MCP side effects inpostMCPSideEffect.This method handles the deletion scenarios for schema metadata and status aspects. It's well-structured and uses stream operations effectively. However, the complexity within the nested flatMap operations requires thorough testing to ensure that deletions are handled correctly without unintended side effects.
260-287: Review the methodbuildSchemaFieldKeyMCPs.This overloaded method directly handles the creation of schema field key MCPs. The method is concise and uses the Java Stream API effectively. However, ensure that the
getFields()call onschemaMetadatais guaranteed to be non-null to avoid potentialNullPointerExceptions.metadata-io/src/main/java/com/linkedin/metadata/entity/ebean/EbeanAspectDao.java (1)
515-517: Enhanced aspect filtering logic added.The new conditional logic introduced at lines 515-517 allows for filtering based on multiple aspect names. This is a significant improvement as it increases the flexibility and capability of the
streamAspectBatchesmethod.datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/system/schemafield/GenerateSchemaFieldsFromSchemaMetadataStep.java (6)
32-52: Documentation clarity and potential future work noted.The class documentation is clear and outlines the responsibilities and operations of the class. It also mentions a future upgrade for generating additional aspects like tags and documentation. This is good for future planning and maintainability.
64-78: Constructor initialization and logging.The constructor properly initializes class fields and logs the initialization. This is good practice for debugging and verifying that the component starts as expected.
91-103: Environment-based skipping logic.The
skipmethod uses an environment variable to determine if the upgrade step should be skipped. This is a flexible approach to control the execution based on deployment environments. Ensure that the environment variable is documented and managed properly in deployment configurations.
105-107: Utility method to get upgrade ID URN.The method
getUpgradeIdUrnuses a static method fromBootstrapStepto generate a URN for the upgrade step. This encapsulates the URN generation logic well.
109-181: Main executable logic of the upgrade step.This method is the core of the upgrade step, handling the streaming, processing, and ingestion of schema aspects. It includes error handling and batch processing logic. The use of lambdas and streams is appropriate and modern. However, ensure that the error handling is robust enough to handle specific exceptions that might occur during batch processing.
85-89: Method to generate URN-like strings for filtering.The
getUrnLikemethod generates a URN-like string for filtering datasets. This is a good use of a helper method to encapsulate functionality. However, ensure that the URN format aligns with expected standards across the DataHub platform.Verification successful
URN format in
getUrnLikemethod is consistent with the expected standards.The format "urn:li:<entity_name>:%" used in the
getUrnLikemethod aligns with the common URN format observed across the codebase. The '%' wildcard is appropriately used for filtering purposes.
- The format "urn:li:<entity_type>:<entity_specific_details>" is consistently used across the DataHub platform.
- The
getUrnLikemethod's format follows this pattern, ensuring compatibility and consistency.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the URN format used in getUrnLike method. # Test: Search for URN format usage in other parts of the codebase. Expect: Consistent URN format. rg --type java $'urn:li:'Length of output: 295980
metadata-models/src/main/resources/entity-registry.yml (1)
457-457: Addition ofstatusaspect toschemaFieldentity.The addition of the
statusaspect to theschemaFieldentity is consistent with the PR's goal to handle schema fields more comprehensively. This change should be verified for compatibility with existing data models and usage in the platform.li-utils/src/main/java/com/linkedin/metadata/Constants.java (1)
85-85: Addition ofSCHEMA_FIELD_KEY_ASPECTconstant.The addition of the
SCHEMA_FIELD_KEY_ASPECTconstant is straightforward and follows the established pattern in the file. This constant likely facilitates the handling of schema field keys across different parts of the application. Ensure that this new constant is used consistently where applicable.Verification successful
Addition of
SCHEMA_FIELD_KEY_ASPECTconstant verified.The
SCHEMA_FIELD_KEY_ASPECTconstant is used appropriately in theSchemaFieldSideEffectclass and its corresponding test class. There are no other relevant places where this constant should be used but isn't.
metadata-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.javametadata-io/src/test/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffectTest.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `SCHEMA_FIELD_KEY_ASPECT` constant across the project. # Test: Search for references to the `SCHEMA_FIELD_KEY_ASPECT` constant. Expect: Consistent usage and definitions. rg --type java $'SCHEMA_FIELD_KEY_ASPECT'Length of output: 2214
Script:
#!/bin/bash # Description: Verify if there are any other relevant files or classes that should be using the `SCHEMA_FIELD_KEY_ASPECT` constant but aren't. # Test: Search for schema field key usage across the project to identify any potential places where the constant should be used. rg --type java 'schemaFieldKey'Length of output: 926
metadata-io/src/test/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffectTest.java (5)
47-76: Review of class-level constants and setup forSchemaFieldSideEffectTestThe constants and configurations defined here are crucial for setting up the test environment. The use of
List.ofwithChangeTypevalues and the configuration ofAspectPluginConfigare appropriate and correctly implemented. TheTEST_PLUGIN_CONFIGis particularly well-setup, specifying class names, enabling the plugin, and defining supported operations and entity aspects.However, it's important to ensure that these configurations align with the actual capabilities and expectations of the
SchemaFieldSideEffectclass in production. This would typically require cross-verifying with the class's documentation or implementation to ensure consistency.
81-91: Setup method reviewThe
setupmethod is well-structured, using mocking effectively to isolate the test environment. The use ofRetrieverContext.builder()to assemble the context is clean and follows good practice in test setup design. This setup method ensures that each test starts with a fresh context, which is crucial for test reliability.
169-348: Test method:statusToSchemaFieldStatusTestand subsequent casesThis method tests the handling of status updates and their effects on
SchemaFieldStatus. Similar to the previous test, it uses loops and conditional checks to validate the behavior under different scenarios. The reset ofmockAspectRetriever(line 177) is crucial for ensuring that mock behavior from previous tests does not interfere.The structure of the test, including the setup of expected results and the use of assertions to validate these results, is consistent and appropriate. The handling of different
ChangeTypescenarios ensures comprehensive coverage.
350-465: Deletion tests:schemaMetadataDeleteTestandstatusDeleteTestThese tests verify the correct behavior when schema metadata and status aspects are deleted. The construction of expected results and the use of assertions (lines 390-401 and 457-465) are well-implemented. These tests are crucial for ensuring that deletions are handled correctly, especially in a system where cascading effects can occur.
The use of
List.ofand loops to construct expected results is efficient and makes the tests easier to understand and maintain.
468-475: Utility method:getTestSchemaMetadataThis method provides a mocked
SchemaMetadataobject for testing. The use ofGenericRecordUtils.deserializeAspectto deserialize a JSON string into aSchemaMetadataobject is a good use of existing utilities. Ensure that the JSON string accurately represents the structure expected by the tests, as any discrepancy here could lead to false positives or negatives in the test outcomes.
.../com/linkedin/datahub/upgrade/system/schemafield/GenerateSchemaFieldsFromSchemaMetadata.java
Show resolved
Hide resolved
...com/linkedin/datahub/upgrade/schemafield/GenerateSchemaFieldsFromSchemaMetadataStepTest.java
Show resolved
Hide resolved
...com/linkedin/datahub/upgrade/schemafield/GenerateSchemaFieldsFromSchemaMetadataStepTest.java
Show resolved
Hide resolved
...a-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.java
Outdated
Show resolved
Hide resolved
...a-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.java
Outdated
Show resolved
Hide resolved
...a-io/src/main/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffect.java
Outdated
Show resolved
Hide resolved
.../src/test/java/com/linkedin/metadata/schemafields/sideeffects/SchemaFieldSideEffectTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SchemaFieldsFromSchemaMetadataConfig.java (1 hunks)
- metadata-service/configuration/src/main/resources/application.yaml (1 hunks)
Additional comments not posted (4)
datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SchemaFieldsFromSchemaMetadataConfig.java (2)
3-7: Review imports and package declaration.The imports and package declaration appear to be correct and relevant to the functionality being implemented.
14-16: Check class annotations.The
@Configurationand@Conditionalannotations are appropriately used here to define a configuration class and conditionally load it based on the system's state.metadata-service/configuration/src/main/resources/application.yaml (2)
344-348: Review new configuration parameters for schemaFieldsFromSchemaMetadata.The configuration parameters for
schemaFieldsFromSchemaMetadataare correctly formatted and seem to be logically placed within the system update section. Ensure that these values are documented and that there are corresponding validations in the application to prevent misconfiguration.
Line range hint
350-352: Review new configuration parameters for structuredProperties.The
structuredPropertiesconfiguration is added correctly and includes parameters for enabling and configuring structured properties. Ensure that these settings are used consistently throughout the application and are well-documented.Verification successful
Review new configuration parameters for structuredProperties.
The
structuredPropertiesconfiguration parameters (enabled,writeEnabled, andsystemUpdateEnabled) are used consistently in the codebase:
UpdateIndicesService.java:
structuredProperties.enabled:@Value("${structuredProperties.enabled}") private boolean _structuredPropertiesHookEnabled;structuredProperties.writeEnabled:@Value("${structuredProperties.writeEnabled}") private boolean _structuredPropertiesWriteEnabled;
ElasticSearchIndexBuilderFactory.java:
structuredProperties.systemUpdateEnabled:@Value("#{new Boolean('${structuredProperties.systemUpdateEnabled}')}") private boolean enableStructuredPropertiesReindex;These parameters are integrated correctly within the application's services, ensuring that the structured properties feature is configurable via the application configuration.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of structuredProperties configuration in the application. # Test: Search for the usage of structuredProperties configuration. Expect: Consistent and documented usage in the application. rg --type yaml -A 5 $'structuredProperties'Length of output: 11310
Script:
#!/bin/bash # Description: Verify the usage of structuredProperties configuration parameters in the codebase. # Test: Search for the usage of structuredProperties.enabled in the codebase. rg 'structuredProperties.enabled' # Test: Search for the usage of structuredProperties.writeEnabled in the codebase. rg 'structuredProperties.writeEnabled' # Test: Search for the usage of structuredProperties.systemUpdateEnabled in the codebase. rg 'structuredProperties.systemUpdateEnabled'Length of output: 560
Script:
#!/bin/bash # Description: Extract the relevant code snippets for structuredProperties configuration parameters. # Extract the usage of structuredProperties.enabled in UpdateIndicesService.java ast-grep --lang java --pattern $'@Value("${structuredProperties.enabled}") $_' metadata-io/src/main/java/com/linkedin/metadata/service/UpdateIndicesService.java # Extract the usage of structuredProperties.writeEnabled in UpdateIndicesService.java ast-grep --lang java --pattern $'@Value("${structuredProperties.writeEnabled}") $_' metadata-io/src/main/java/com/linkedin/metadata/service/UpdateIndicesService.java # Extract the usage of structuredProperties.systemUpdateEnabled in ElasticSearchIndexBuilderFactory.java ast-grep --lang java --pattern $'@Value("#{new Boolean(\'${structuredProperties.systemUpdateEnabled}\')}") $_' metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.javaLength of output: 555
Script:
#!/bin/bash # Description: Extract the relevant code snippets for structuredProperties configuration parameters using rg with context lines. # Extract the usage of structuredProperties.enabled in UpdateIndicesService.java rg 'structuredProperties.enabled' -A 5 metadata-io/src/main/java/com/linkedin/metadata/service/UpdateIndicesService.java # Extract the usage of structuredProperties.writeEnabled in UpdateIndicesService.java rg 'structuredProperties.writeEnabled' -A 5 metadata-io/src/main/java/com/linkedin/metadata/service/UpdateIndicesService.java # Extract the usage of structuredProperties.systemUpdateEnabled in ElasticSearchIndexBuilderFactory.java rg 'structuredProperties.systemUpdateEnabled' -A 5 metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.javaLength of output: 1089
.../src/main/java/com/linkedin/datahub/upgrade/config/SchemaFieldsFromSchemaMetadataConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SchemaFieldsFromSchemaMetadataConfig.java (1 hunks)
- metadata-service/configuration/src/main/resources/application.yaml (1 hunks)
Additional comments not posted (4)
datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SchemaFieldsFromSchemaMetadataConfig.java (2)
3-7: Review imports and package declaration.The imports and package declaration appear to be correct and relevant to the functionality being implemented.
14-16: Check class annotations.The
@Configurationand@Conditionalannotations are appropriately used here to define a configuration class and conditionally load it based on the system's state.metadata-service/configuration/src/main/resources/application.yaml (2)
344-348: Review new configuration parameters for schemaFieldsFromSchemaMetadata.The configuration parameters for
schemaFieldsFromSchemaMetadataare correctly formatted and seem to be logically placed within the system update section. Ensure that these values are documented and that there are corresponding validations in the application to prevent misconfiguration.
Line range hint
350-352: Review new configuration parameters for structuredProperties.The
structuredPropertiesconfiguration is added correctly and includes parameters for enabling and configuring structured properties. Ensure that these settings are used consistently throughout the application and are well-documented.Verification successful
Review new configuration parameters for structuredProperties.
The
structuredPropertiesconfiguration parameters (enabled,writeEnabled, andsystemUpdateEnabled) are used consistently in the codebase:
UpdateIndicesService.java:
structuredProperties.enabled:@Value("${structuredProperties.enabled}") private boolean _structuredPropertiesHookEnabled;structuredProperties.writeEnabled:@Value("${structuredProperties.writeEnabled}") private boolean _structuredPropertiesWriteEnabled;
ElasticSearchIndexBuilderFactory.java:
structuredProperties.systemUpdateEnabled:@Value("#{new Boolean('${structuredProperties.systemUpdateEnabled}')}") private boolean enableStructuredPropertiesReindex;These parameters are integrated correctly within the application's services, ensuring that the structured properties feature is configurable via the application configuration.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of structuredProperties configuration in the application. # Test: Search for the usage of structuredProperties configuration. Expect: Consistent and documented usage in the application. rg --type yaml -A 5 $'structuredProperties'Length of output: 11310
Script:
#!/bin/bash # Description: Verify the usage of structuredProperties configuration parameters in the codebase. # Test: Search for the usage of structuredProperties.enabled in the codebase. rg 'structuredProperties.enabled' # Test: Search for the usage of structuredProperties.writeEnabled in the codebase. rg 'structuredProperties.writeEnabled' # Test: Search for the usage of structuredProperties.systemUpdateEnabled in the codebase. rg 'structuredProperties.systemUpdateEnabled'Length of output: 560
Script:
#!/bin/bash # Description: Extract the relevant code snippets for structuredProperties configuration parameters. # Extract the usage of structuredProperties.enabled in UpdateIndicesService.java ast-grep --lang java --pattern $'@Value("${structuredProperties.enabled}") $_' metadata-io/src/main/java/com/linkedin/metadata/service/UpdateIndicesService.java # Extract the usage of structuredProperties.writeEnabled in UpdateIndicesService.java ast-grep --lang java --pattern $'@Value("${structuredProperties.writeEnabled}") $_' metadata-io/src/main/java/com/linkedin/metadata/service/UpdateIndicesService.java # Extract the usage of structuredProperties.systemUpdateEnabled in ElasticSearchIndexBuilderFactory.java ast-grep --lang java --pattern $'@Value("#{new Boolean(\'${structuredProperties.systemUpdateEnabled}\')}") $_' metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.javaLength of output: 555
Script:
#!/bin/bash # Description: Extract the relevant code snippets for structuredProperties configuration parameters using rg with context lines. # Extract the usage of structuredProperties.enabled in UpdateIndicesService.java rg 'structuredProperties.enabled' -A 5 metadata-io/src/main/java/com/linkedin/metadata/service/UpdateIndicesService.java # Extract the usage of structuredProperties.writeEnabled in UpdateIndicesService.java rg 'structuredProperties.writeEnabled' -A 5 metadata-io/src/main/java/com/linkedin/metadata/service/UpdateIndicesService.java # Extract the usage of structuredProperties.systemUpdateEnabled in ElasticSearchIndexBuilderFactory.java rg 'structuredProperties.systemUpdateEnabled' -A 5 metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.javaLength of output: 1089
Comments failed to post (1)
datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SchemaFieldsFromSchemaMetadataConfig.java
18-34: Ensure proper validation of configuration values.
The configuration values
batchSize,delayMs, andlimitare used directly within theGenerateSchemaFieldsFromSchemaMetadataandGenerateSchemaFieldsFromSchemaMetadataStepclasses without explicit validation. This could lead to potential runtime issues if these values are incorrectly set.
- File:
datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/system/schemafield/GenerateSchemaFieldsFromSchemaMetadata.java- File:
datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/system/schemafield/GenerateSchemaFieldsFromSchemaMetadataStep.javaRecommendation: Add validation logic for these configuration parameters to ensure they are within acceptable ranges or formats before using them in the application.
Analysis chain
Review the bean definition method.
This method is well-defined with appropriate parameter annotations and types. It correctly creates a
NonBlockingSystemUpgradebean using theGenerateSchemaFieldsFromSchemaMetadataclass. However, ensure that all the configurations such asbatchSize,delayMs, andlimitare being used correctly in the system and that their values are validated somewhere in the application to prevent configuration errors.Ensure that the configuration values are validated appropriately to avoid runtime issues due to incorrect settings.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper usage of configuration values in the application. # Test: Search for the usage of configuration values. Expect: Proper validation or usage in the system. rg --type java -A 5 $'systemUpdate.schemaFieldsFromSchemaMetadata'Length of output: 2092
Script:
#!/bin/bash # Description: Search for the instantiation and method invocations of GenerateSchemaFieldsFromSchemaMetadata to verify usage and validation of configuration values. # Find where GenerateSchemaFieldsFromSchemaMetadata is instantiated rg --type java 'new GenerateSchemaFieldsFromSchemaMetadata' # Find where methods of GenerateSchemaFieldsFromSchemaMetadata are invoked rg --type java 'GenerateSchemaFieldsFromSchemaMetadata'Length of output: 3349
Script:
#!/bin/bash # Description: Extract the constructor and methods of GenerateSchemaFieldsFromSchemaMetadata to verify usage and validation of configuration values. # Extract the constructor and methods of GenerateSchemaFieldsFromSchemaMetadata ast-grep --lang java --pattern 'class GenerateSchemaFieldsFromSchemaMetadata { public GenerateSchemaFieldsFromSchemaMetadata($_, $_, $_, $_, $_, $_, $_) { $$$ } $$$ }'Length of output: 179
Script:
#!/bin/bash # Description: Extract the class definition and methods of GenerateSchemaFieldsFromSchemaMetadata to verify usage and validation of configuration values. # Search for the class definition of GenerateSchemaFieldsFromSchemaMetadata rg --type java 'class GenerateSchemaFieldsFromSchemaMetadata' -A 20 # Search for methods within GenerateSchemaFieldsFromSchemaMetadata rg --type java 'GenerateSchemaFieldsFromSchemaMetadata' -A 10Length of output: 31083
Script:
#!/bin/bash # Description: Search for validation logic within the GenerateSchemaFieldsFromSchemaMetadataStep class. # Search for validation logic within GenerateSchemaFieldsFromSchemaMetadataStep rg --type java 'GenerateSchemaFieldsFromSchemaMetadataStep' -A 20Length of output: 25493
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
a836ea3 to
0692047
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SchemaFieldsFromSchemaMetadataConfig.java (1 hunks)
- metadata-service/configuration/src/main/resources/application.yaml (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- datahub-upgrade/src/main/java/com/linkedin/datahub/upgrade/config/SchemaFieldsFromSchemaMetadataConfig.java
- metadata-service/configuration/src/main/resources/application.yaml
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
0692047 to
945f0cd
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
945f0cd to
114f71d
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
actionlint
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:11:51: Double quote to prevent globbing and word splitting [shellcheck]
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:2:55: Double quote to prevent globbing and word splitting [shellcheck]
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:2:45: Double quote to prevent globbing and word splitting [shellcheck]
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:2:45: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 107 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:1:118: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 548 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:1:128: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 589 in 114f71d
[actionlint] reported by reviewdog 🐶
missing input "image_tag" which is required by action "Custom Docker build and push" defined at "./.github/actions/docker-custom-build-and-push". all required inputs are "image_tag", "images" [action]
datahub/.github/workflows/docker-unified.yml
Line 613 in 114f71d
[actionlint] reported by reviewdog 🐶
input "tags" is not defined in action "Custom Docker build and push" defined at "./.github/actions/docker-custom-build-and-push". available inputs are "build-args", "context", "file", "flavor", "image_tag", "images", "password", "platforms", "publish", "target", "username" [action]
datahub/.github/workflows/docker-unified.yml
Line 618 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:1:123: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 630 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:1:123: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 684 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:1:113: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 770 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:2:55: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 812 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:4:63: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 812 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:6:95: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 812 in 114f71d
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:8:24: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 812 in 114f71d
[actionlint] reported by reviewdog 🐶
property "integrations_service_change" is not defined in object type {backend_change: string; backend_only: string; branch_name: string; docker-login: string; elasticsearch_setup_change: string; frontend_change: string; frontend_only: string; full_tag: string; ingestion_base_change: string; ingestion_change: string; ingestion_only: string; kafka_setup_change: string; mysql_setup_change: string; postgres_setup_change: string; pr-publish: string; publish: string; python_release_version: string; repository_name: string; slim_tag: string; smoke_test_change: string; tag: string; unique_full_tag: string; unique_slim_tag: string; unique_tag: string} [expression]
datahub/.github/workflows/docker-unified.yml
Line 927 in 114f71d
[actionlint] reported by reviewdog 🐶
property "short_sha" is not defined in object type {backend_change: string; backend_only: string; branch_name: string; docker-login: string; elasticsearch_setup_change: string; frontend_change: string; frontend_only: string; full_tag: string; ingestion_base_change: string; ingestion_change: string; ingestion_only: string; kafka_setup_change: string; mysql_setup_change: string; postgres_setup_change: string; pr-publish: string; publish: string; python_release_version: string; repository_name: string; slim_tag: string; smoke_test_change: string; tag: string; unique_full_tag: string; unique_slim_tag: string; unique_tag: string} [expression]
datahub/.github/workflows/docker-unified.yml
Line 1066 in 114f71d
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
114f71d to
dcb59e9
Compare
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
actionlint
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:1:113: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 770 in dcb59e9
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:2:55: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 812 in dcb59e9
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:4:63: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 812 in dcb59e9
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:6:95: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 812 in dcb59e9
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:8:24: Double quote to prevent globbing and word splitting [shellcheck]
datahub/.github/workflows/docker-unified.yml
Line 812 in dcb59e9
[actionlint] reported by reviewdog 🐶
property "integrations_service_change" is not defined in object type {backend_change: string; backend_only: string; branch_name: string; docker-login: string; elasticsearch_setup_change: string; frontend_change: string; frontend_only: string; full_tag: string; ingestion_base_change: string; ingestion_change: string; ingestion_only: string; kafka_setup_change: string; mysql_setup_change: string; postgres_setup_change: string; pr-publish: string; publish: string; python_release_version: string; repository_name: string; slim_tag: string; smoke_test_change: string; tag: string; unique_full_tag: string; unique_slim_tag: string; unique_tag: string} [expression]
datahub/.github/workflows/docker-unified.yml
Line 927 in dcb59e9
[actionlint] reported by reviewdog 🐶
property "short_sha" is not defined in object type {backend_change: string; backend_only: string; branch_name: string; docker-login: string; elasticsearch_setup_change: string; frontend_change: string; frontend_only: string; full_tag: string; ingestion_base_change: string; ingestion_change: string; ingestion_only: string; kafka_setup_change: string; mysql_setup_change: string; postgres_setup_change: string; pr-publish: string; publish: string; python_release_version: string; repository_name: string; slim_tag: string; smoke_test_change: string; tag: string; unique_full_tag: string; unique_slim_tag: string; unique_tag: string} [expression]
datahub/.github/workflows/docker-unified.yml
Line 1066 in dcb59e9
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
dcb59e9 to
d5d0fd6
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
d5d0fd6 to
a46fe59
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
a46fe59 to
5b7e517
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
5b7e517 to
60f0f8b
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
60f0f8b to
d8f7f7b
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
d8f7f7b to
49876d5
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
49876d5 to
c987986
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
c987986 to
9869c17
Compare
david-leifker
force-pushed
the
schema-field-side-effect
branch
from
9869c17 to
637c562
Compare
.github/workflows/docker-unified.yml
Outdated
There was a problem hiding this comment.
[actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects [shellcheck]
.github/workflows/docker-unified.yml
Outdated
There was a problem hiding this comment.
[actionlint] reported by reviewdog 🐶
property "integrations_service_change" is not defined in object type {backend_change: string; backend_only: string; branch_name: string; docker-login: string; elasticsearch_setup_change: string; frontend_change: string; frontend_only: string; full_tag: string; ingestion_base_change: string; ingestion_change: string; ingestion_only: string; kafka_setup_change: string; mysql_setup_change: string; postgres_setup_change: string; pr-publish: string; publish: string; python_release_version: string; repository_name: string; slim_tag: string; smoke_test_change: string; tag: string; unique_full_tag: string; unique_slim_tag: string; unique_tag: string} [expression]
datahub/.github/workflows/docker-unified.yml
Line 939 in 637c562
.github/workflows/docker-unified.yml
Outdated
There was a problem hiding this comment.
[actionlint] reported by reviewdog 🐶
property "short_sha" is not defined in object type {backend_change: string; backend_only: string; branch_name: string; docker-login: string; elasticsearch_setup_change: string; frontend_change: string; frontend_only: string; full_tag: string; ingestion_base_change: string; ingestion_change: string; ingestion_only: string; kafka_setup_change: string; mysql_setup_change: string; postgres_setup_change: string; pr-publish: string; publish: string; python_release_version: string; repository_name: string; slim_tag: string; smoke_test_change: string; tag: string; unique_full_tag: string; unique_slim_tag: string; unique_tag: string} [expression]
datahub/.github/workflows/docker-unified.yml
Line 1078 in 637c562
Maintain schemaField entities and status aspects from dataset's schemaMetadata and status aspect.
schemaFieldAliasesand SideEffect MCP generation to hold v1/v2 schemaField URNsEnables schemaField Side Effect
MCP_SIDE_EFFECTS_SCHEMA_FIELD_ENABLED=true
Creates schemaField entities from Dataset schemaMetadata
SYSTEM_UPDATE_SCHEMA_FIELDS_FROM_SCHEMA_METADATA_ENABLED=true
Enable the new document id algorithm for schemaFields
SYSTEM_UPDATE_SCHEMA_FIELDS_DOC_IDS_ENABLED=true
Update All SchemaField ES Documents with Hash Algorithm
ELASTICSEARCH_INDEX_DOC_IDS_SCHEMA_FIELD_HASH_ID_ENABLED=true
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
AspectsBatchclass.Tests
Chores