Formalize @Batched and @BatchesGroupedBy annotations for batching #334
Conversation
…ble to be accessible in output logic
WalkthroughThis pull request introduces a range of refactorings and updates across multiple modules. A new VS Code Java setting for automatic null analysis and a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant VCG as VajramCodeGenerator
participant CV as ComputeVajram
participant IO as IOVajram
participant BP as Batch Processor
U->>VCG: Request execution
VCG->>CV: Initiate batched execution
CV-->>VCG: Return compute result
VCG->>IO: Invoke IO execution
IO-->>VCG: Return IO result
VCG->>BP: Process _batchItems
BP-->>VCG: Return processed batches
VCG-->>U: Send combined response
sequenceDiagram
participant F as FacetJavaType
participant FF as Facet Field
F->>FF: Retrieve Mandatory annotation via getAnnotation()
FF-->>F: Return annotation details
F->>F: Process and validate annotation
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/FacetJavaType.java (2)
75-75: LGTM! Consider adding null checks for defensive programming.The change to use standard Java reflection API for annotation access is good. However, consider adding null checks for
facetField()to make the code more robust.- Mandatory mandatory = facet.facetField().getAnnotation(Mandatory.class); + Mandatory mandatory = Optional.ofNullable(facet.facetField()) + .map(field -> field.getAnnotation(Mandatory.class)) + .orElse(null);
110-110: LGTM! Apply consistent null checks here as well.The change maintains consistency with the previous modification. Consider applying the same defensive programming approach here.
- Mandatory mandatory = facet.facetField().getAnnotation(Mandatory.class); + Mandatory mandatory = Optional.ofNullable(facet.facetField()) + .map(field -> field.getAnnotation(Mandatory.class)) + .orElse(null);vajram/vajram-samples/src/main/java/com/flipkart/krystal/vajram/samples/greeting/UserService.java (1)
44-45: Consider using a more descriptive parameter name.While the type changes look good, the parameter name
modInputsis less descriptive than the previous name. Consider usingbatchItemsto maintain consistency with the naming incallUserService.- private static CompletableFuture<Map<UserService_BatchItem, UserInfo>> batchServiceCall( - Collection<UserService_BatchItem> modInputs) { + private static CompletableFuture<Map<UserService_BatchItem, UserInfo>> batchServiceCall( + Collection<UserService_BatchItem> batchItems) {vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/FacetGenModel.java (1)
53-67: Consider improving exception handling and performance.While the implementation works, there are several areas for improvement:
- Replace @SneakyThrows with explicit exception handling
- Add validation for the Class.forName() call
- Consider caching the annotation lookups to avoid repeated reflection
Here's a suggested improvement:
- @SneakyThrows - public default List<Annotation> getAnnotations() { + public default List<Annotation> getAnnotations() { List<Annotation> annotations = new ArrayList<>(); for (AnnotationMirror annotationMirror : facetField().getAnnotationMirrors()) { TypeElement element = (TypeElement) annotationMirror.getAnnotationType().asElement(); - @SuppressWarnings("unchecked") - Class<? extends Annotation> annotationType = - (Class<? extends Annotation>) Class.forName(element.toString()); - Annotation annotation = facetField().getAnnotation(annotationType); - if (annotation != null) { - annotations.add(annotation); + try { + @SuppressWarnings("unchecked") + Class<? extends Annotation> annotationType = + (Class<? extends Annotation>) Class.forName(element.toString()); + Annotation annotation = facetField().getAnnotation(annotationType); + if (annotation != null) { + annotations.add(annotation); + } + } catch (ClassNotFoundException e) { + // Log warning and continue + System.err.println("Warning: Could not load annotation class " + element); } } return annotations; }vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/VajramCodeGenerator.java (1)
599-702: Improve error messages for facet validation.The facet validation logic is correct but could benefit from more descriptive error messages.
Apply this diff to enhance error messages:
- throw util.errorAndThrow( - "No facet with the name " - + param.getSimpleName() - + " exists in the vajram " - + vajramInfo.vajramId(), - param); + throw util.errorAndThrow( + String.format( + "Facet '%s' not found in vajram '%s'. Please ensure the facet is properly defined.", + param.getSimpleName(), + vajramInfo.vajramId()), + param);vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/Mandatory.java (1)
49-52: Fix typo in Javadoc.There's a typo in the Javadoc: "ahy" should be "any".
Apply this diff to fix the typo:
- * value whose type is ahy of array, collection, map, or strings, is unset. + * value whose type is any of array, collection, map, or strings, is unset.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.vscode/settings.json(1 hunks)config/checker/stubs/jdk.astub(1 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/Constants.java(2 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/DeclaredTypeVisitor.java(0 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/FacetJavaType.java(2 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/Utils.java(2 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/VajramCodeGenerator.java(10 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/DependencyModel.java(0 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/FacetGenModel.java(2 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/GivenFacetModel.java(0 hunks)vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/VajramInfo.java(0 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/AbstractVajram.java(0 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/BatchableVajram.java(0 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/ComputeVajram.java(1 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/IOVajram.java(1 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/Vajram.java(1 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/batching/BatchEnabledFacetValues.java(1 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/batching/Batched.java(2 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/batching/BatchesGroupedBy.java(1 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/ApplicableToTypes.java(1 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/InputSource.java(0 hunks)vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/Mandatory.java(1 hunks)vajram/vajram-krystex/src/main/java/com/flipkart/krystal/vajramexecutor/krystex/InputBatcherConfig.java(1 hunks)vajram/vajram-krystex/src/main/java/com/flipkart/krystal/vajramexecutor/krystex/VajramKryonGraph.java(2 hunks)vajram/vajram-krystex/src/test/java/com/flipkart/krystal/vajramexecutor/krystex/test_vajrams/friendsservice/FriendsService.java(2 hunks)vajram/vajram-krystex/src/test/java/com/flipkart/krystal/vajramexecutor/krystex/test_vajrams/userservice/TestUserService.java(3 hunks)vajram/vajram-samples/src/main/java/com/flipkart/krystal/vajram/samples/calculator/adder/Adder.java(2 hunks)vajram/vajram-samples/src/main/java/com/flipkart/krystal/vajram/samples/greeting/UserService.java(2 hunks)
💤 Files with no reviewable changes (7)
- vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/InputSource.java
- vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/BatchableVajram.java
- vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/DeclaredTypeVisitor.java
- vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/AbstractVajram.java
- vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/DependencyModel.java
- vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/VajramInfo.java
- vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/GivenFacetModel.java
✅ Files skipped from review due to trivial changes (2)
- .vscode/settings.json
- vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/Constants.java
👮 Files not reviewed due to content moderation or server errors (2)
- vajram/vajram-krystex/src/main/java/com/flipkart/krystal/vajramexecutor/krystex/InputBatcherConfig.java
- vajram/vajram-krystex/src/main/java/com/flipkart/krystal/vajramexecutor/krystex/VajramKryonGraph.java
🔇 Additional comments (45)
config/checker/stubs/jdk.astub (1)
27-30: LGTM! Good addition for null safety.The new
Objectsclass stub withrequireNonNullmethod is correctly defined and properly annotated with@NonNull. This addition will help the Checker Framework perform more accurate null analysis, particularly in theVajramCodeGeneratorclass where it's being used.vajram/vajram-krystex/src/main/java/com/flipkart/krystal/vajramexecutor/krystex/InputBatcherConfig.java (3)
379-379: LGTM! Simplified BatcherContext record.The removal of the
vajramfield fromBatcherContextreduces coupling and aligns with the broader refactoring to remove theBatchableVajraminterface.
379-379: LGTM! Record simplification improves design.The removal of the
vajramfield fromBatcherContextreduces coupling and aligns with the broader refactor to removeBatchableVajram. The simplified record now only contains the essentialLogicDecoratorContextfield.
379-379: LGTM! Simplified BatcherContext record.The removal of the
BatchableVajramfield fromBatcherContextaligns with the broader changes in the codebase where theBatchableVajraminterface has been deleted. The simplified record now only contains the essentialLogicDecoratorContextfield.vajram/vajram-krystex/src/main/java/com/flipkart/krystal/vajramexecutor/krystex/VajramKryonGraph.java (5)
133-137: LGTM! Updated type check and error message.The changes correctly:
- Update the type check from
BatchableVajramtoIOVajram.- Improve the error message to accurately reflect the new type requirement.
- Simplify the
BatcherContextcreation by removing thevajramparameter.These changes align well with the broader refactoring to formalize batching annotations.
Also applies to: 158-158
133-136: LGTM! Type check update aligns with design changes.The type check update from
BatchableVajramtoIOVajramis consistent with the broader refactor to formalize batching annotations. The error message has been updated accordingly.
158-158: LGTM! BatcherContext creation simplified.The
BatcherContextcreation has been simplified to match the record's new structure, which now only requires theLogicDecoratorContextparameter.
133-137: LGTM! Updated type check to IOVajram.The type check has been correctly updated to use
IOVajraminstead ofBatchableVajram, and the error message has been updated accordingly. This change aligns with the broader refactor across the codebase.
158-158: LGTM! Simplified BatcherContext creation.The
BatcherContextcreation has been simplified to only pass thedecoratorContext, which aligns with the removal of theBatchableVajramfield from theBatcherContextrecord.vajram/vajram-krystex/src/test/java/com/flipkart/krystal/vajramexecutor/krystex/test_vajrams/friendsservice/FriendsService.java (4)
7-7: LGTM!The imports align with the PR objectives of formalizing batching annotations and support the new collection type used in the method signature.
Also applies to: 11-11
23-23: LGTM!The annotation change from
@Batchto@Batchedaligns with the PR objectives while maintaining the existing@Mandatoryand@Inputannotations.
29-30: LGTM!The method signature changes improve the API by:
- Using the more generic
ImmutableCollectioninstead ofImmutableList- Adopting clearer naming with
BatchIteminstead ofBatchElem- Using a parameter name that better reflects its type
32-33: LGTM!The implementation correctly adapts to use the new
BatchItemtype while maintaining the same logical flow.vajram/vajram-krystex/src/test/java/com/flipkart/krystal/vajramexecutor/krystex/test_vajrams/userservice/TestUserService.java (4)
10-10: LGTM! Import updated for the new @Batched annotation.The import change aligns with the PR objective of formalizing the @Batched annotation.
14-14: LGTM! Import added for ImmutableCollection.Using ImmutableCollection instead of ImmutableList provides more flexibility while maintaining immutability guarantees.
27-27: LGTM! Updated to @Batched annotation.The annotation change aligns with the formalization of batching annotations while maintaining the correct combination with @mandatory and @input annotations.
37-46: LGTM! Method signature and implementation updated for batch processing.The changes improve the API by:
- Using more flexible ImmutableCollection type
- Adopting consistent BatchItem terminology
- Using clearer variable naming with _batchItems
vajram/vajram-samples/src/main/java/com/flipkart/krystal/vajram/samples/greeting/UserService.java (3)
10-10: LGTM!The new imports are correctly added to support the transition to the new
@Batchedannotation and collection types.Also applies to: 14-15
24-24: LGTM!The annotation change from
@Batchto@Batchedaligns with the PR's objective to formalize batching annotations.
30-31: LGTM!The changes improve flexibility by using
ImmutableCollectionand maintain consistency with the new batch item terminology.vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/models/FacetGenModel.java (3)
5-6: LGTM!The new imports are correctly added to support the batching annotations and utility functions.
Also applies to: 9-13, 15-15
39-41: LGTM!The simplified implementation directly checks for the @mandatory annotation, which is cleaner and more maintainable.
45-51: LGTM!The new batching methods are well-implemented:
- Consistent with the PR's objective of formalizing batching annotations
- Follow the same pattern as
isMandatory()- Properly marked as
defaultmethodsvajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/batching/BatchesGroupedBy.java (1)
1-11: LGTM!The
BatchesGroupedByannotation is correctly defined as a marker annotation for field-level usage with runtime retention.vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/batching/Batched.java (1)
3-23: LGTM!The
Batchedannotation is well-documented and correctly defined as a marker annotation for field-level usage with runtime retention. The documentation clearly explains the batching mechanism and its performance benefits.vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/VajramCodeGenerator.java (4)
40-40: LGTM! Import changes align with the new batching annotations.The imports have been updated to include
requireNonNullfor null checks and new batching annotations (Batched,BatchesGroupedBy).Also applies to: 70-72
582-587: LGTM! Conditional logic for batch execution is clear and well-structured.The code properly handles both batched and non-batched execution paths with clear braces for better readability.
1734-1736: LGTM! Improved facet filtering using method references.The code now uses method references for filtering facets, making it more concise and readable.
2008-2008: LGTM! Updated annotation check fromBatchtoBatched.The code correctly checks for the new
Batchedannotation instead of the oldBatchannotation.vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/batching/BatchEnabledFacetValues.java (1)
8-8:✅ Verification successful
LGTM! Method renaming improves terminology consistency.
The renaming from
_batchElement()to_batchItem()aligns with the broader terminology changes in the codebase, making it more consistent and clearer.Let's verify the impact of this renaming across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old method name # and verify that all implementations use the new name # Check for any remaining references to _batchElement echo "Checking for remaining references to _batchElement..." rg "_batchElement" # Find all implementations of BatchEnabledFacetValues echo -e "\nChecking implementations of BatchEnabledFacetValues..." ast-grep --pattern 'class $_ implements $$$BatchEnabledFacetValues$$$'Length of output: 333
LGTM! Method renaming verified across the codebase.
All references to the deprecated
_batchElement()have been successfully replaced with_batchItem(). The automated search confirmed there are no lingering references to the old method name and that implementations ofBatchEnabledFacetValuesare correctly using the new nomenclature.
- Confirmed no instances of
_batchElement()remain.- Verified consistency in the interface implementation.
vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/ComputeVajram.java (2)
26-38: LGTM! Implementation maintains quality and safety.The implementation demonstrates good practices:
- Uses immutable collections
- Handles nullability properly
- Maintains type safety
- Uses functional programming effectively
- Has clear separation of concerns
24-24:✅ Verification successful
LGTM! Verify interface implementation completeness.
The change from extending
AbstractVajram<T>to implementingVajram<T>directly simplifies the class hierarchy while maintaining extensibility through thenon-sealedmodifier.Let's verify that all required interface methods are implemented:
🏁 Script executed:
#!/bin/bash # Description: Check Vajram interface methods and their implementation in ComputeVajram # Find Vajram interface methods echo "Vajram interface methods:" ast-grep --pattern 'interface Vajram<$_> { $$$ $method($_) { $$$ } $$$ }' # Find ComputeVajram implementations echo -e "\nComputeVajram implementations:" ast-grep --pattern 'class ComputeVajram<$_> implements Vajram<$_> { $$$ $method($_) { $$$ } $$$ }'Length of output: 338
LGTM – The updated class declaration is correct.
The shift from extending
AbstractVajram<T>to directly implementingVajram<T>while remaining non-sealed simplifies the inheritance hierarchy. SinceComputeVajramis abstract, it’s acceptable that it does not provide concrete implementations of all interface methods—the expectation is for concrete subclasses to implement them. Please ensure that the required interface methods are properly implemented in the concrete subclasses.vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/IOVajram.java (1)
25-25:❓ Verification inconclusive
LGTM! The simplified class hierarchy looks good.
The direct implementation of
Vajram<T>aligns with interface-based design principles and reduces complexity in the class hierarchy.Let's verify the impact of removing the base classes. Run this script to check for any shared functionality that might need to be reimplemented:
🏁 Script executed:
#!/bin/bash # Description: Check for shared functionality from removed base classes that might need reimplementation # Search for usages of methods from AbstractVajram ast-grep --pattern 'class $_ extends AbstractVajram { $$$ }' # Search for implementations of BatchableVajram methods ast-grep --pattern 'class $_ implements BatchableVajram { $$$ }'Length of output: 133
Simplified Hierarchy Verified – Manual Cross-Check Recommended
LGTM overall. The refactor to have
IOVajram<T>implementVajram<T>directly streamlines the class hierarchy in line with interface-based design. While our automated searches (via bothast-grepandrg) did not yield any references to shared functionality from the removed base classes (AbstractVajramandBatchableVajram), please verify manually that any functionality previously provided by these base classes is either re-implemented or no longer required.
- Actionable Check: Manually confirm that no methods or shared logic from
AbstractVajramandBatchableVajramhave been inadvertently omitted from the new design.vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/ApplicableToTypes.java (1)
13-25: LGTM!The annotation elements are well-documented with clear explanations of the behavior for empty array cases.
vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/Mandatory.java (2)
15-18: LGTM!The annotation metadata is correctly configured with appropriate retention policy, target, and documentation.
25-54: LGTM!The type applicability annotations are correctly applied to enum constants, providing clear type constraints for each default behavior.
vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/Vajram.java (3)
15-54: Documentation is comprehensive and well-aligned with the new structure.The documentation effectively explains the Vajram paradigm, including:
- Clear categorization of Vajram types
- Detailed explanation of facets and logic types
- Future extensibility plans
57-71: Method definitions follow best practices.The interface methods demonstrate good design principles:
- Use of immutable collections for thread safety
- Proper null safety annotations
- Clear separation of concerns
55-55:❓ Verification inconclusive
Significant architectural change in interface hierarchy.
The modification of the
permitsclause represents a substantial architectural shift:
- Removal of the abstraction layer (
AbstractVajram)- Direct permission of concrete implementations (
ComputeVajram,IOVajram)- Elimination of the dedicated batching interface (
BatchableVajram)While this simplifies the hierarchy, please verify that:
- All existing implementations have been updated
- The removal of
AbstractVajramdoesn't lead to code duplication- The batching functionality is properly migrated to the new structure
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the removed classes echo "Searching for AbstractVajram references..." rg "AbstractVajram" echo "Searching for BatchableVajram references..." rg "BatchableVajram" # Search for existing implementations to ensure they're updated echo "Searching for Vajram implementations..." ast-grep --pattern 'class $_ implements Vajram'Length of output: 1191
🏁 Script executed:
#!/bin/bash # Verify that permitted implementations exist in the codebase echo "Searching for ComputeVajram implementations..." rg "class ComputeVajram" echo "Searching for IOVajram implementations..." rg "class IOVajram"Length of output: 549
Architectural Change Verified – Action Needed on Documentation
The change in the
permitsclause fromAbstractVajram, BatchableVajramtoComputeVajram, IOVajramhas been validated:
- Implementation Check: Both
ComputeVajramandIOVajramimplementations exist as expected.- Code Search: No active code references remain for
AbstractVajramorBatchableVajram(they appear only in resource files).- Documentation Artifacts: The Excalidraw diagrams still reference
AbstractVajramandBatchableVajram. If these resources are intended to illustrate current architecture, please update them accordingly.vajram/vajram-samples/src/main/java/com/flipkart/krystal/vajram/samples/calculator/adder/Adder.java (4)
10-11: LGTM!The imports are correctly added and align with the changes in the code.
Also applies to: 15-15
27-27: LGTM!The field annotations are correctly updated to use the new formalized batching annotations.
Also applies to: 30-30, 34-37
40-42: LGTM!The constants are correctly moved and maintain their functionality.
44-46: LGTM!The method signature and implementation are correctly updated to use the new batch item types and collection interface.
Also applies to: 51-51
vajram/vajram-codegen/src/main/java/com/flipkart/krystal/vajram/codegen/Utils.java (2)
402-402: LGTM!The dependency model builder is correctly refactored to use method chaining for improved readability.
782-782: LGTM!The mandatory annotation handling is correctly simplified by directly accessing the annotation from the facet field.
vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/ApplicableToTypes.java
Show resolved
Hide resolved
vajram/vajram-java-sdk/src/main/java/com/flipkart/krystal/vajram/facets/ApplicableToTypes.java
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Refactor
Chores