[WIP] #215 | Add soft-deletion support for Varadhi Resources #218
Conversation
- Add Javadoc comments to the Constants class and its inner classes. - Add private constructors to inner classes to prevent instantiation.
- Added unit tests for VaradhiTopicService, covering topic creation, deletion (hard and soft), restoration, and existence checks. - Implemented tests for VaradhiTopic entity to verify its creation and status updates. - Added tests for VaradhiTopicHandler to ensure correct handling of topic related requests. - Refactored existing tests to reduce code duplication by extracting common setup logic into helper methods. - Ensured comprehensive coverage of various edge cases, such as filtering out inactive topics and handling meta store failures.
WalkthroughThis pull request refactors and enhances several Varadhi components. Core constants and classes (including lifecycle and deletion types) are renamed and documented more clearly. New enums, records, and nested classes increase type safety and maintainability. Functionality for soft and hard deletion—with restoration support—is introduced in both topics and subscriptions. Multiple service, handler, and factory classes are updated alongside extensive test refactoring to ensure consistency and improved error handling. A new PATCH route method is also added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SubscriptionService
participant MetaStore
Client->>SubscriptionService: deleteSubscription(subscriptionName, deletionType=SOFT_DELETE)
SubscriptionService->>SubscriptionService: Validate state & mark as inactive
SubscriptionService->>MetaStore: Update subscription record
MetaStore-->>SubscriptionService: Confirmation
SubscriptionService-->>Client: Soft deletion acknowledged
Client->>SubscriptionService: restoreSubscription(subscriptionName)
SubscriptionService->>MetaStore: Update subscription record to ACTIVE
MetaStore-->>SubscriptionService: Confirmation
SubscriptionService-->>Client: Restoration acknowledged
Possibly related issues
Poem
Tip 🌐 Web search-backed reviews and chat
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: 5
🔭 Outside diff range comments (1)
server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (1)
Line range hint
64-92: Create subscription partial rollback
While the code handles exceptions, consider rolling back the newly created subscription if provisioning permanently fails.
🧹 Nitpick comments (37)
entities/src/main/java/com/flipkart/varadhi/entities/VaradhiSubscription.java (2)
167-172: Restoration logic.
Restoring the subscription to CREATED is clear, but consider whether a new “RESTORED” state might improve traceability.
174-190: Consider supplying a default or optional for missing property values.
Throwing an exception is valid, though returning an optional or fallback could be more user-friendly in certain scenarios.common/src/main/java/com/flipkart/varadhi/Constants.java (5)
23-23: Consider making DefaultTopicCapacity immutable.The
DefaultTopicCapacityconstant could potentially be modified sinceTopicCapacityPolicymight be mutable.Consider making
TopicCapacityPolicyimmutable or creating an unmodifiable instance:- public static final TopicCapacityPolicy DefaultTopicCapacity = new TopicCapacityPolicy(100, 400, 2); + public static final TopicCapacityPolicy DefaultTopicCapacity = Collections.unmodifiableObject(new TopicCapacityPolicy(100, 400, 2));
53-54: Enhance documentation for query parameters.While the parameters are self-explanatory, adding documentation about valid values and usage would be helpful, especially for
deletionTypewhich is crucial for the new deletion feature.Add parameter documentation:
+ /** Specifies the type of deletion operation (e.g., "soft", "hard") */ public static final String QUERY_PARAM_DELETION_TYPE = "deletionType"; + /** When true, allows deletion even if there are dependent resources */ public static final String QUERY_PARAM_IGNORE_CONSTRAINTS = "ignoreConstraints";
83-93: Consider using standard HTTP status code constants.Instead of defining custom HTTP status code constants, consider using
HttpStatusfrom Spring Framework or similar standard libraries if available in the project.- public static final int HTTP_RATE_LIMITED = 429; - public static final int HTTP_UNPROCESSABLE_ENTITY = 422; + // Use org.springframework.http.HttpStatus if Spring is available + public static final int HTTP_RATE_LIMITED = HttpStatus.TOO_MANY_REQUESTS.value(); + public static final int HTTP_UNPROCESSABLE_ENTITY = HttpStatus.UNPROCESSABLE_ENTITY.value();
96-118: Consider expanding metrics structure for deletion feature.Since this PR implements deletion functionality, consider adding corresponding metrics.
Example structure:
public static class Deletion { public static final String SOFT_DELETION_METER = "deletion.soft"; public static final String HARD_DELETION_METER = "deletion.hard"; public static final String RESTORATION_METER = "deletion.restore"; private Deletion() { // Private constructor to prevent instantiation } }
Line range hint
126-129: Document rationale for size limits.The maximum sizes for payload and headers would benefit from documentation explaining their rationale.
Add explanatory comments:
+ // 5MB max payload size to balance between large messages and server resource usage public static final int PAYLOAD_SIZE_MAX = 5 * 1024 * 1024; + // Limited number of headers to prevent header explosion attacks public static final int HEADERS_ALLOWED_MAX = 10; + // Header size limits aligned with common proxy server defaults public static final int HEADER_NAME_SIZE_MAX = 64; public static final int HEADER_VALUE_SIZE_MAX = 256;server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java (4)
45-48: Consider moving test constants to a separate class.These constants could be reused across other test classes. Consider moving them to a dedicated test constants class for better maintainability and reusability.
67-69: Consider using@ExtendWith(MockitoExtension.class).Instead of manually calling
MockitoAnnotations.openMocks(this), you can use the@ExtendWith(MockitoExtension.class)annotation at the class level for a more idiomatic JUnit 5 approach.+@ExtendWith(MockitoExtension.class) class VaradhiTopicServiceTest { // ... @BeforeEach public void setUp() { - MockitoAnnotations.openMocks(this); varadhiTopicFactory = spy(new VaradhiTopicFactory(storageTopicFactory, REGION, DEFAULT_CAPACITY_POLICY)); // ... }
162-179: Enhance topic-in-use test with specific error message assertion.The test verifies the exception type but doesn't validate the error message. Add an assertion to verify the specific error message for better test coverage.
Exception exception = assertThrows( InvalidOperationForResourceException.class, () -> varadhiTopicService.delete(varadhiTopic.getName(), ResourceDeletionType.HARD_DELETE) ); verify(metaStore, never()).deleteTopic(varadhiTopic.getName()); assertEquals(InvalidOperationForResourceException.class, exception.getClass()); +assertEquals("Cannot delete topic " + varadhiTopic.getName() + " as it has active subscriptions", exception.getMessage());
323-330: Add input validation tosetupMockTopics.The method assumes that
topicNamesandtopicStatuseslists have the same size. Add validation to prevent potentialIndexOutOfBoundsException.private void setupMockTopics(String projectName, List<String> topicNames, List<Boolean> topicStatuses) { + if (topicNames.size() != topicStatuses.size()) { + throw new IllegalArgumentException("Topic names and statuses lists must have the same size"); + } when(metaStore.getTopicNames(projectName)).thenReturn(topicNames); for (int i = 0; i < topicNames.size(); i++) { // ... } }server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (6)
15-17: Expanded class-level documentation
The Javadoc succinctly describes the service’s purpose. Consider mentioning soft/hard deletion capabilities for completeness.
52-67: Active status check
ThrowingResourceNotFoundExceptionfor inactive topics is sensible. Logging the inactive status could improve debugging.
69-70: Method documentation
Comments are clear about topic deletion. Optionally referencing the newResourceDeletionTypeparam would further clarify usage.
88-96: Soft delete concurrency
Marking a topic inactive is straightforward. If multiple soft-delete requests arrive concurrently, consider whether extra idempotency checks are needed.
117-134: Restore method
Good flow: checks if active, throws if not deleted, updates status to ACTIVE, and saves. Optionally log a final success message.
136-154: Validation performance
Scanning all subscription names might affect performance at scale. Consider more direct checks or indexing in future iterations.server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (5)
116-146: Update subscription logic
Revertinggroupedmid-operation can cause confusion. Consider a clearer approach to reduce potential state flip-flops.
157-186: Delete subscription
Hard vs. soft deletion is clearly distinguished. Validate concurrency if a subscription is in a transitional state (e.g., provisioning).
235-251:getValidatedSubscriptionduplication
Ensures subscription validity. Consider unifying all state checks in one method to minimize code duplication.
283-302:performSubscriptionOperation
A neat approach to reduce duplication. Watch for concurrency issues if subscription states change rapidly.
327-335: handleSoftDelete
Straightforward approach. You might add an additional check for already-inactive subscriptions.server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (3)
144-176: Hierarchy construction
Generating subscription and topic hierarchies is straightforward. Consider caching repeated calls toprojectService.getProjectif performance becomes a concern.
181-185: List endpoint
Fetching all subscription names might be expensive for large projects. Consider pagination or search filters if needed.
204-218: Create endpoint
UsesvaradhiSubscriptionFactoryand checks the subscribed topic. Adding more explicit error logs on creation failure might help troubleshooting.entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java (2)
6-9: Add documentation for enum values.Consider adding Javadoc for each enum value to clearly document their intended use and behavior.
public enum ResourceDeletionType { + /** Marks the resource as inactive without removing it from the database */ SOFT_DELETE, + /** Permanently removes the resource from the database */ HARD_DELETE, + /** Default deletion type, typically maps to SOFT_DELETE for safety */ DEFAULT;
18-27: Enhance string parsing robustness.The string parsing could be more robust by:
- Using a constant for the default value
- Adding case-insensitive matching
+ /** Default deletion type used when input is invalid */ + private static final ResourceDeletionType DEFAULT_TYPE = DEFAULT; + public static ResourceDeletionType fromValue(String value) { if (value == null || value.isBlank()) { - return DEFAULT; + return DEFAULT_TYPE; } - return switch (value.trim().toUpperCase()) { - case "SOFT_DELETE" -> SOFT_DELETE; - case "HARD_DELETE" -> HARD_DELETE; - default -> DEFAULT; + try { + return valueOf(value.trim().toUpperCase()); + } catch (IllegalArgumentException e) { + return DEFAULT_TYPE; + } }spi/src/main/java/com/flipkart/varadhi/spi/db/MetaStore.java (1)
52-52: Add method documentation.Add Javadoc for the new
updateTopicmethod to document its purpose, parameters, and any exceptions it might throw.+ /** + * Updates an existing topic in the meta store. + * + * @param topic the topic to update + * @throws IllegalArgumentException if the topic is null + * @throws ResourceNotFoundException if the topic doesn't exist + */ void updateTopic(VaradhiTopic topic);entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (2)
20-20: Consider making status field final.The status field should be made final to ensure immutability, with status changes handled through creating new instances.
- private Status status; + private final Status status;
105-110: Add status transition validation.Consider adding validation for status transitions to prevent invalid state changes (e.g., INACTIVE to INACTIVE).
public void updateStatus(Status newStatus) { if (newStatus == null) { throw new IllegalArgumentException("Status cannot be null"); } + if (this.status == newStatus) { + throw new IllegalArgumentException("Topic is already in " + newStatus + " state"); + } this.status = newStatus; }entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java (2)
109-118: Enhance status transition test coverage.Consider adding parameterized tests for status transitions and edge cases.
+ @ParameterizedTest + @CsvSource({ + "ACTIVE,INACTIVE", + "INACTIVE,ACTIVE" + }) + void updateStatus_ValidTransitions_Succeeds(Status from, Status to) { + VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false); + varadhiTopic.updateStatus(from); + + varadhiTopic.updateStatus(to); + + assertEquals(to, varadhiTopic.getStatus()); + } + + @Test + void updateStatus_SameState_ThrowsException() { + VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false); + + assertThrows(IllegalArgumentException.class, + () -> varadhiTopic.updateStatus(Status.ACTIVE), + "Updating to same state should throw exception"); + }
27-29: Consider adding validation in helper method.The helper method should validate input parameters.
private VaradhiTopic createDefaultVaradhiTopic(boolean grouped) { + Objects.requireNonNull(PROJECT_NAME, "Project name cannot be null"); + Objects.requireNonNull(TOPIC_NAME, "Topic name cannot be null"); + Objects.requireNonNull(TOPIC_CAPACITY, "Topic capacity cannot be null"); return VaradhiTopic.of(PROJECT_NAME, TOPIC_NAME, grouped, TOPIC_CAPACITY); }server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (1)
166-171: Enhance test coverage for deletion types.The test verifies the basic deletion functionality but doesn't test different deletion types. Consider adding test cases for both soft and hard deletion scenarios.
Apply this diff to add test coverage for different deletion types:
- @Test - void testSubscriptionDelete() throws InterruptedException { - HttpRequest<Buffer> request = createRequest(HttpMethod.DELETE, getSubscriptionUrl("sub1", project)); - SubscriptionResource resource = getSubscriptionResource("sub1", project, topicResource); - - doReturn(project).when(projectService).getCachedProject(project.getName()); - ArgumentCaptor<String> captorSubName = ArgumentCaptor.forClass(String.class); - ArgumentCaptor<Project> captorProject = ArgumentCaptor.forClass(Project.class); - doReturn(CompletableFuture.completedFuture(null)).when(subscriptionService) - .deleteSubscription(captorSubName.capture(), captorProject.capture(), any(), any()); - - sendRequestWithoutBody(request, null); - assertEquals(captorSubName.getValue(), resource.getSubscriptionInternalName()); - assertEquals(captorProject.getValue().getName(), project.getName()); - verify(subscriptionService, times(1)).deleteSubscription(any(), any(), any(), any()); - } + @Test + void testSubscriptionSoftDelete() throws InterruptedException { + verifyDeleteRequest("SOFT_DELETE", ResourceDeletionType.SOFT_DELETE); + } + + @Test + void testSubscriptionHardDelete() throws InterruptedException { + verifyDeleteRequest("HARD_DELETE", ResourceDeletionType.HARD_DELETE); + } + + @Test + void testSubscriptionDeleteDefaultType() throws InterruptedException { + verifyDeleteRequest(null, ResourceDeletionType.SOFT_DELETE); + } + + private void verifyDeleteRequest(String deletionType, ResourceDeletionType expectedDeletionType) + throws InterruptedException { + String url = getSubscriptionUrl("sub1", project); + if (deletionType != null) { + url += "?deletionType=" + deletionType; + } + + HttpRequest<Buffer> request = createRequest(HttpMethod.DELETE, url); + SubscriptionResource resource = getSubscriptionResource("sub1", project, topicResource); + + doReturn(project).when(projectService).getCachedProject(project.getName()); + ArgumentCaptor<String> captorSubName = ArgumentCaptor.forClass(String.class); + ArgumentCaptor<Project> captorProject = ArgumentCaptor.forClass(Project.class); + ArgumentCaptor<ResourceDeletionType> captorDeletionType = ArgumentCaptor.forClass(ResourceDeletionType.class); + + doReturn(CompletableFuture.completedFuture(null)).when(subscriptionService) + .deleteSubscription(captorSubName.capture(), captorProject.capture(), any(), captorDeletionType.capture()); + + sendRequestWithoutBody(request, null); + + assertEquals(captorSubName.getValue(), resource.getSubscriptionInternalName()); + assertEquals(captorProject.getValue().getName(), project.getName()); + assertEquals(expectedDeletionType, captorDeletionType.getValue()); + verify(subscriptionService, times(1)).deleteSubscription(any(), any(), any(), any()); + }server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java (1)
217-229: Improve error message clarity.The error message could be more descriptive to help users understand the issue better.
Apply this diff to enhance the error message:
- throw new IllegalArgumentException("Project name in URL and request body do not match."); + throw new IllegalArgumentException( + String.format("Project name mismatch: '%s' in URL does not match '%s' in request body.", + projectName, topicResource.getProject()) + );server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (2)
169-183: Add test coverage for error scenarios.Consider adding tests for:
- Non-existent topic
- Invalid topic name format
- Topic in deleted state
Would you like me to provide the implementation for these test cases?
231-238: Add test coverage for restore error scenarios.Consider adding tests for:
- Restoring a non-existent topic
- Restoring an active topic
- Restoring a hard-deleted topic
Would you like me to provide the implementation for these test cases?
server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (1)
Line range hint
349-365: Enhance test helpers and mock setup for deletion scenarios.To improve test maintainability and reduce code duplication for the new deletion scenarios, consider:
- Adding a helper method for deletion operations:
private CompletableFuture<Void> deleteSubscription(String name, ResourceDeletionType deletionType) { return subscriptionService.deleteSubscription( name, o1t1p1, requestedBy, deletionType); }
- Enhancing mock setup to verify deletion type handling:
@Test void verifyDeletionTypeHandling() { // Setup doReturn(unGroupedTopic).when(varadhiMetaStore).getTopic(unGroupedTopic.getName()); subscriptionService.createSubscription(unGroupedTopic, sub1, o1t1p1); // Verify that deletion type is correctly passed to the meta store deleteSubscription(sub1.getName(), ResourceDeletionType.SOFT_DELETE); verify(varadhiMetaStore).markSubscriptionAsDeleted( eq(sub1.getName()), eq(ResourceDeletionType.SOFT_DELETE)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
common/src/main/java/com/flipkart/varadhi/Constants.java(3 hunks)entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/VaradhiSubscription.java(4 hunks)entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java(1 hunks)producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java(1 hunks)server/src/main/java/com/flipkart/varadhi/config/RestOptions.java(1 hunks)server/src/main/java/com/flipkart/varadhi/db/VaradhiMetaStore.java(1 hunks)server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java(2 hunks)server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java(4 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java(4 hunks)server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java(2 hunks)spi/src/main/java/com/flipkart/varadhi/spi/db/MetaStore.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java
- server/src/main/java/com/flipkart/varadhi/config/RestOptions.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-workflow-docker-build / build
- GitHub Check: build
🔇 Additional comments (79)
entities/src/main/java/com/flipkart/varadhi/entities/VaradhiSubscription.java (19)
9-11: Good documentation block.
No issues found with this introductory subscription description.
28-29: Constants naming is clear.
Defining these error messages as constants improves readability.
31-46: Comprehensive constructor documentation.
The parameter descriptions are clear and informative.
62-63: Well-structured constructor validations.
Using helper methods to validate fields ensures consistency with minimal duplication.Also applies to: 69-71
74-89: Factory method documentation is clear.
The static creation pattern is well-defined and self-explanatory.
104-104: Initial status assignment.
Automatically setting the status to CREATING aligns with a typical creation workflow.
108-116: Confirm the definition of "well-provisioned."
Currently, only the CREATED state is considered well-provisioned. Verify whether additional states should also be regarded as well-provisioned.
118-125: Clarify the meaning of "active."
Right now, all states except INACTIVE are treated as active. Confirm if states like CREATE_FAILED or DELETE_FAILED should also be non-active.
128-134: Creation failure handling.
Marking the subscription as CREATE_FAILED with a specific message is a robust approach to surface details.
137-141: “Created” status transition.
Straightforward status update, keeping the code more readable.
144-150: Delete failure handling looks good.
Clean usage of the shared update flow to manage failure messages.
153-158: “Deleting” status transition.
This matches the typical lifecycle. No issues noted.
160-164: Inactive status transition.
Explicitly marking the subscription as INACTIVE is consistent with soft-deletion semantics.
192-207: Validation method is properly encapsulated.
This reduces duplication across the class.
209-223: Shards validation.
This logic is correct for ensuring a nonzero shard count.
225-239: Properties validation.
Properly handles empty or null properties, ensuring consistent usage.
241-249: Update status concurrency check.
If multiple threads can modify the subscription simultaneously, concurrency considerations may be necessary. Otherwise, this is fine.
252-261: State enum definitions.
Naming is intuitive, supporting typical lifecycle transitions.
264-273: Status class usage.
Well-structured representation for storing both the state and accompanying message.common/src/main/java/com/flipkart/varadhi/Constants.java (2)
32-46: LGTM! Well-structured constants class.The PathParams class follows best practices with:
- Clear documentation
- Logical grouping of related constants
- Prevention of instantiation
73-73: Verify impact of TAG_REMOTE_HOST renaming.The renaming from TAG_REMOTEHOST to TAG_REMOTE_HOST improves naming consistency but might affect existing code.
server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java (4)
280-308: LGTM!The project-specific topic tests are well-structured and thoroughly cover both active and inactive topic scenarios.
79-120: Add test for duplicate topic creation.The creation tests cover successful creation and various failure scenarios, but there's no test for attempting to create a topic that already exists. This is an important edge case to verify.
Consider adding a test like:
@Test void createVaradhiTopic_TopicAlreadyExists_ThrowsException() { VaradhiTopic varadhiTopic = createVaradhiTopicMock(); when(metaStore.checkTopicExists(varadhiTopic.getName())).thenReturn(true); assertThrows(ResourceAlreadyExistsException.class, () -> varadhiTopicService.create(varadhiTopic, project)); verify(storageTopicService, never()).create(any(), any()); verify(metaStore, never()).createTopic(any()); }
207-231: Add test for restoring a hard-deleted topic.The restoration tests don't cover the scenario of attempting to restore a topic that has been hard-deleted. This edge case should be verified to ensure proper error handling.
Consider adding a test like:
@Test void restoreVaradhiTopic_HardDeletedTopic_ThrowsException() { String topicName = "default.testTopic"; when(metaStore.getTopic(topicName)).thenThrow(new ResourceNotFoundException("Topic not found")); Exception exception = assertThrows( ResourceNotFoundException.class, () -> varadhiTopicService.restore(topicName) ); assertEquals("Topic not found", exception.getMessage()); verify(metaStore, never()).updateTopic(any()); }
255-278: Add test for retrieving a soft-deleted topic.The retrieval tests should verify that soft-deleted topics can still be retrieved but are marked as inactive. This is important for the soft deletion feature.
Consider adding a test like:
@Test void getVaradhiTopic_SoftDeletedTopic_ReturnsInactiveTopic() { VaradhiTopic varadhiTopic = createVaradhiTopicMock(); varadhiTopic.updateStatus(VaradhiTopic.Status.INACTIVE); when(metaStore.getTopic(varadhiTopic.getName())).thenReturn(varadhiTopic); VaradhiTopic retrievedTopic = varadhiTopicService.get(varadhiTopic.getName()); assertNotNull(retrievedTopic); assertFalse(retrievedTopic.isActive()); }✅ Verification successful
Review comment is correct - test for retrieving soft-deleted topics is needed
The codebase shows that VaradhiTopic supports active/inactive states through its Status enum, and while the entity layer has good test coverage for status changes, the service layer (VaradhiTopicServiceTest) lacks tests for retrieving topics in different states. The suggested test would improve coverage of this important functionality.
The proposed test in the review comment is well-structured and correctly uses:
- Status.INACTIVE for marking the topic as soft-deleted
- Proper assertions to verify the inactive state
- Consistent style with existing tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for VaradhiTopic class definition and Status enum ast-grep --pattern 'class VaradhiTopic { $$$ }' # Search for any existing soft deletion related tests rg -l "soft.*delet|inactive|status" --type java # Search for status-related methods in VaradhiTopicService ast-grep --pattern 'class VaradhiTopicService { $$$ }'Length of output: 2735
Script:
#!/bin/bash # Check VaradhiTopic implementation echo "=== VaradhiTopic.java ===" cat entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java echo -e "\n=== TopicState.java ===" cat entities/src/main/java/com/flipkart/varadhi/entities/TopicState.java echo -e "\n=== VaradhiTopicTest.java ===" cat entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.javaLength of output: 10319
server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (5)
4-8: Imports introduced for resource and exception handling
These imports (ResourceDeletionType, StorageTopic, VaradhiTopic, and exception classes) correctly reflect the new deletion logic and exception usage.
21-21: Well-defined final fields
DeclaringstorageTopicServiceandmetaStoreas final is good practice to ensure immutability.
24-31: Constructor JavaDocs
The constructor documentation is clear and aligns well with the parameters.
72-86: Deletion Type logic
Soft vs. hard deletion logic is separated cleanly. The TODO for namespace/tenant cleanup might need follow-up to avoid confusion.
157-176: Existence check & topic retrieval
TheexistsandgetVaradhiTopicsmethods are clear. Filtering out inactive topics helps maintain a clean API.server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (11)
7-7: ResourceNotFoundException import
This import aligns well with usage throughout the class for invalid/missing subscriptions.
14-14: BiFunction usage
Neat approach passing subscription operations as a BiFunction, promoting clean invocation.
16-18: Class-level Javadoc
Documentation gives a good overview of subscription service responsibilities and new exception handling.
25-35: Constructor clarity
The Javadoc thoroughly describes each constructor parameter.
41-51: Listing subscriptions
Filtering byisActiveOrWellProvisionedByNameis intuitive for returning valid subscriptions.
54-61: Retrieval approach
The method delegates togetValidatedSubscriptionfor streamlined error handling.
93-112: Start/Stop subscription
Delegating toperformSubscriptionOperationis effective for code reuse and consistency.
188-211: Restore subscription
Restores an inactive subscription. The flow is simple, and the log statement is helpful.
213-233: State checks
isActiveOrWellProvisionedhelps unify subscription readiness checks.
253-266: Grouped subscription check
Enforces synergy between grouped topics and grouped subscriptions.
268-276: Version update check
Throws exception on version mismatch to prevent inconsistent updates. Solid concurrency safeguard.server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (17)
5-8: Reintroduced VaradhiTopicService and ResourceType imports
These imports restore the handling of topics and resource authorization.
25-29: New query param constants
UsingQUERY_PARAM_DELETION_TYPEandQUERY_PARAM_IGNORE_CONSTRAINTSstandardizes these features across endpoints.
36-39: Class Javadoc
Describes the subscription operations comprehensively, including delete/restore functionality.
43-61: Constructor
Instantiating property validators and default value providers up front is clean.
Line range hint
78-120: Route definitions
AddingRestoreSubscriptionroute aligns well with soft deletion. Overall route structure remains consistent.
192-196: Get endpoint
Creates aSubscriptionResourcefrom a validated subscription. Straightforward flow.
222-241: Update endpoint
TheTODOmentions splitting into separate update operations. Carefully evaluate partial updates to avoid unintended changes.
244-262: Delete endpoint
Defaults to soft deletion if not specified. Safer default choice.
265-273: Restore endpoint
Complements soft deletion. The REST route is consistent with the existing pattern.
275-291: Start/Stop endpoints
Delegation tosubscriptionServiceis straightforward. Ensures consistent lifecycle management in service logic.
293-304: Building subscription FQN
A concise, well-defined helper method.
307-322: Validating subscription resource
Multiple validations in a single method. Centralizes error checking well.
326-335: Super user constraints
RestrictingignoreConstraintsto super admins prevents abuse.
338-347: Retry policy validation
Restricting to a specific retry count can simplify operational assumptions.
350-360: Project consistency check
Ensures the path-param project matches the request body project, preventing misconfiguration.
362-386: Get subscribed topic
Constructs the FQN for the topic and retrieves it. Straightforward approach.
388-418: Property validation
Ensures unknown properties are rejected, sets defaults, and validates syntax. This is robust and user-friendly.server/src/main/java/com/flipkart/varadhi/db/VaradhiMetaStore.java (1)
193-197: LGTM!The implementation follows the established pattern in the class and correctly updates the topic data in the ZK meta store.
server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java (13)
3-6: LGTM!The new imports are correctly organized and necessary for the soft-deletion functionality.
35-38: LGTM!The class-level Javadoc is clear and follows standard format.
46-52: LGTM!The constructor Javadoc is well-documented with clear parameter descriptions.
Line range hint
63-92: LGTM!The route definitions are well-organized and the new restoration route follows the established pattern.
94-102: LGTM!The method rename improves clarity and the implementation is correct.
104-127: LGTM!The method correctly handles resource hierarchies for both request types.
129-137: LGTM!The method is well-documented and correctly implements topic retrieval.
145-146: Address TODO comments.Consider implementing:
- Vertx ValidationHandlers for request body validation
- Rollback mechanisms for failure scenarios
Would you like me to help implement these improvements?
164-177: LGTM!The method correctly implements deletion type handling with a sensible default to soft deletion.
179-187: LGTM!The restore method is well-documented and correctly implements topic restoration.
189-201: LGTM!The method efficiently uses streams for topic filtering and mapping.
204-215: LGTM!The method is well-documented and correctly uses the helper method for name construction.
231-240: LGTM!The method is well-documented and correctly implements topic name construction.
server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (6)
44-82: LGTM!The test class setup is well-organized with proper use of Mockito annotations and constants.
84-111: LGTM!The route setup is comprehensive and well-organized.
123-166: LGTM!The create topic tests provide good coverage of success and error scenarios.
186-208: LGTM!The list topics tests provide good coverage of different scenarios.
212-229: LGTM!The delete topic tests provide excellent coverage of all deletion scenarios.
241-270: LGTM!The helper methods are well-organized and promote code reuse.
server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (1)
350-351: Add test coverage for soft deletion and restoration.While the hard deletion test case is implemented, comprehensive test coverage is needed for the new soft deletion functionality. Consider adding the following test cases:
- Soft deletion of a subscription
- Restoration of a soft-deleted subscription
- Edge cases:
- Attempting to restore a hard-deleted subscription
- Attempting to hard delete an already soft-deleted subscription
- Attempting to soft delete an already soft-deleted subscription
✅ Verification successful
Add test coverage for soft deletion and restoration functionality
The codebase has proper implementation of soft deletion and restoration features in
SubscriptionService, butSubscriptionServiceTestlacks test coverage for these scenarios. Following the patterns established inVaradhiTopicServiceTest, add test cases for:
- Soft deletion of a subscription
- Successful restoration of a soft-deleted subscription
- Edge cases:
- Attempting to restore an active subscription
- Attempting to restore a hard-deleted subscription
- Attempting to soft delete an already soft-deleted subscription
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing test coverage of soft deletion and restoration. # Test: Search for test methods related to soft deletion and restoration rg -t java "test.*(?i)(soft.*delete|restore)" "server/src/test" # Test: Search for usage of SOFT_DELETE enum value in tests rg -t java "SOFT_DELETE" "server/src/test"Length of output: 762
Script:
#!/bin/bash # Search for restore/undelete related methods and endpoints rg -t java "(?i)(restore|undelete)" "server/src" # Search for method signatures in SubscriptionService ast-grep --pattern 'interface SubscriptionService { $$$ }'Length of output: 4499
server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java
Show resolved
Hide resolved
server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java
Show resolved
Hide resolved
server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java
Outdated
Show resolved
Hide resolved
- Overloaded the `makeDeleteRequest` and `makeHttpDeleteRequest` method in `E2EBase` to accept a deletion type parameter. - Updated cleanup methods to use hard delete to ensure resources are fully removed. - Ensured that resources are hard deleted during cleanup to prevent conflicts when creating new ones with the same name. - Corrected exception messages in tests to match the expected responses. - Added `@JsonIgnore` annotation on `isActive` field in `VaradhiTopic` entity to prevent it from being serialized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java (1)
125-125: Review test cleanup strategy.The test cleanup operations consistently use hard delete. Consider documenting this decision in a test utility class or as a comment to explain why hard delete is preferred for test cleanup.
entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (3)
20-41: Add null check in constructor for status parameter.While the
updateStatusmethod includes null validation, the constructor should also validate the status parameter to maintain consistency and prevent null states.private VaradhiTopic( String name, int version, boolean grouped, TopicCapacityPolicy capacity, Map<String, InternalCompositeTopic> internalTopics, Status status ) { super(name, version); + if (status == null) { + throw new IllegalArgumentException("Status cannot be null"); + } this.grouped = grouped; this.capacity = capacity; this.internalTopics = internalTopics; this.status = status; }
43-54: Enhance factory method documentation.The documentation should mention that the topic is created with a default
Status.ACTIVEstate./** * Creates a new VaradhiTopic instance. * * @param project the project associated with the topic * @param name the name of the topic * @param grouped whether the topic is grouped * @param capacity the capacity policy of the topic + * @return a new VaradhiTopic instance with Status.ACTIVE state */
122-128: Consider enhancing Status enum for better lifecycle management.Given that this implements soft-deletion support, consider:
- Adding a
PENDING_DELETIONstate to support graceful deletion process- Implementing state transition validation to prevent invalid state changes
Example enhancement:
public enum Status { ACTIVE, PENDING_DELETION, // Intermediate state during deletion INACTIVE; // Soft-deleted state public boolean canTransitionTo(Status newStatus) { return switch (this) { case ACTIVE -> newStatus == PENDING_DELETION; case PENDING_DELETION -> newStatus == INACTIVE; case INACTIVE -> newStatus == ACTIVE; // Support restoration }; } }Then update the
updateStatusmethod to use this validation:public void updateStatus(Status newStatus) { if (newStatus == null) { throw new IllegalArgumentException("Status cannot be null"); } if (!this.status.canTransitionTo(newStatus)) { throw new IllegalStateException( "Invalid state transition from " + this.status + " to " + newStatus); } this.status = newStatus; }server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java (2)
137-137: Extract "HARD_DELETE" as a constant.The string literal "HARD_DELETE" is used in multiple places. Consider extracting it as a class constant to improve maintainability and prevent typos.
+ private static final String HARD_DELETE = "HARD_DELETE"; - makeDeleteRequest(getTopicsUri(project, topicName), "HARD_DELETE", 200); + makeDeleteRequest(getTopicsUri(project, topicName), HARD_DELETE, 200);
166-167: Improve readability of subscription name parsing.
- This is another occurrence of the "HARD_DELETE" string literal that should use the suggested constant.
- The subscription name parsing logic using split makes the code less readable.
Consider extracting the subscription name parsing:
+ private static String parseSubscriptionName(String fullName) { + return fullName.split(NAME_SEPARATOR_REGEX)[1]; + } static void cleanupSubscriptionsOnProject(Project project) { getSubscriptions(makeListRequest(getSubscriptionsUri(project), 200)).forEach( - s -> makeDeleteRequest( - getSubscriptionsUri(project, s.split(NAME_SEPARATOR_REGEX)[1]), "HARD_DELETE", 200)); + s -> makeDeleteRequest( + getSubscriptionsUri(project, parseSubscriptionName(s)), HARD_DELETE, 200)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java(1 hunks)server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java(4 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java(2 hunks)server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java(5 hunks)server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java(3 hunks)server/src/testE2E/java/com/flipkart/varadhi/TopicTests.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-workflow-docker-build / build
- GitHub Check: build
🔇 Additional comments (12)
server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java (3)
84-84: Consider adding test coverage for soft deletion.While using hard delete for cleanup is valid, this test class should include coverage for both soft and hard deletion scenarios to validate the new functionality completely.
Would you like me to help generate additional test cases to cover soft deletion flows?
195-195: LGTM! Improved error message clarity.The updated error message is more descriptive and clearly indicates that the grouped subscription constraint applies to both creation and updates.
Line range hint
1-197: Add comprehensive test coverage for deletion functionality.While the changes are correct, this test class would benefit from additional test coverage for the new deletion functionality:
- Test soft deletion of a subscription
- Test restoration of a soft-deleted subscription
- Test operations on soft-deleted subscriptions (should fail)
- Test hard deletion after soft deletion
- Test various edge cases around deletion states
Would you like me to help generate these additional test cases?
entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (1)
98-120: Well-implemented status management methods.The implementation includes proper validation, documentation, and appropriate use of @JsonIgnore annotation.
server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java (2)
156-156: Same string literal "HARD_DELETE" used.This is another occurrence of the string literal that should use the suggested constant.
255-258: Extract query parameter name as constant and verify parameter validation.
- The query parameter name "deletionType" should be extracted as a constant for consistency.
- We should verify that the deletionType parameter is properly validated in the endpoint handler.
+ private static final String DELETION_TYPE_PARAM = "deletionType"; static Response makeHttpDeleteRequest(String targetUrl, String deletionType) { return getClient() .target(targetUrl) - .queryParam("deletionType", deletionType) + .queryParam(DELETION_TYPE_PARAM, deletionType) .request(MediaType.APPLICATION_JSON_TYPE) .header(USER_ID_HEADER, SUPER_USER) .delete(); }Let's verify the parameter validation in the endpoint handler:
Also applies to: 302-309
server/src/testE2E/java/com/flipkart/varadhi/TopicTests.java (2)
62-64: LGTM! Improved error message format.The simplified error message format is more user-friendly while maintaining clarity.
67-67: 🛠️ Refactor suggestionAdd test coverage for soft deletion scenarios.
While the hard deletion tests are in place, there are several missing test scenarios that should be covered:
- Soft deletion of topics
- Restoration of soft-deleted topics
- Validation of invalid deletion types
- Error scenarios in deletion flows (e.g., deleting already deleted topics)
Here's a suggested test structure:
@Test public void testTopicDeletionScenarios() { String topicName = "test-topic-deletion"; TopicResource topic = TopicResource.unGrouped(topicName, o1t1Project1.getName(), null); // Create topic makeCreateRequest(getTopicsUri(o1t1Project1), topic, 200); // Test soft deletion makeDeleteRequest(getTopicsUri(o1t1Project1, topicName), "SOFT_DELETE", 200); // Verify topic is soft deleted but still exists TopicResource deletedTopic = makeGetRequest( getTopicsUri(o1t1Project1, topicName), TopicResource.class, 200 ); Assertions.assertTrue(deletedTopic.isDeleted()); // Test restoration makeRestoreRequest(getTopicsUri(o1t1Project1, topicName), 200); TopicResource restoredTopic = makeGetRequest( getTopicsUri(o1t1Project1, topicName), TopicResource.class, 200 ); Assertions.assertFalse(restoredTopic.isDeleted()); // Test invalid deletion type makeDeleteRequest( getTopicsUri(o1t1Project1, topicName), "INVALID_DELETE", 400, "Invalid deletion type specified", true ); // Clean up makeDeleteRequest(getTopicsUri(o1t1Project1, topicName), "HARD_DELETE", 200); }Also applies to: 98-99
server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (4)
239-240: LGTM! Improved error message clarity.The updated error message is more descriptive and accurately reflects both creation and update scenarios.
298-298: LGTM! Enhanced version conflict message.The updated error message is more professional and provides clear guidance on the next steps.
325-326: LGTM! Consistent error messaging.The error message maintains consistency with the creation scenario, helping users understand that the same rules apply to both operations.
352-353: Add test coverage for soft deletion.While the hard deletion test case is implemented, we should also add test coverage for soft deletion to ensure both deletion types work as expected. This aligns with the PR objectives of implementing comprehensive soft deletion support.
Consider adding the following test cases:
- Soft deletion marks the subscription as deleted but keeps it retrievable
- Restoration of a soft-deleted subscription
- Verify that operations on soft-deleted subscriptions are properly restricted
+ @Test + void softDeleteSubscriptionMarksAsDeleted(VertxTestContext ctx) { + Checkpoint checkpoint = ctx.checkpoint(1); + doReturn(unGroupedTopic).when(varadhiMetaStore).getTopic(unGroupedTopic.getName()); + subscriptionService.createSubscription(unGroupedTopic, sub1, o1t1p1); + + String name = sub1.getName(); + CompletableFuture<SubscriptionState> status = + CompletableFuture.completedFuture(SubscriptionState.forStopped()); + doReturn(status).when(controllerRestApi).getSubscriptionState(name, requestedBy); + + Future.fromCompletionStage( + subscriptionService.deleteSubscription(name, o1t1p1, requestedBy, ResourceDeletionType.SOFT_DELETE)) + .onComplete(ctx.succeeding(v -> { + VaradhiSubscription subscription = subscriptionService.getSubscription(name); + assertTrue(subscription.isDeleted()); + checkpoint.flag(); + })); + }✅ Verification successful
Review comment is valid - soft deletion test coverage is missing
The codebase shows that soft deletion is implemented in
SubscriptionHandlers(defaulting toSOFT_DELETE) andSubscriptionServicesupports both deletion types, but test coverage only exists for hard deletion. The suggested test cases would help ensure the soft deletion functionality works as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for soft deletion related tests and usage echo "=== Searching for soft deletion test methods ===" rg -l "test.*soft.*delete" --type java echo -e "\n=== Searching for SOFT_DELETE usage in tests ===" rg "SOFT_DELETE" --type java echo -e "\n=== Searching for soft deletion related assertions ===" rg "isDeleted|softDelete" --type javaLength of output: 1908
Script:
#!/bin/bash # Check subscription service implementation for soft deletion echo "=== Checking SubscriptionService implementation ===" ast-grep --pattern 'deleteSubscription($$$, $_, ResourceDeletionType $_) { $$$ }' echo -e "\n=== Checking for isDeleted method in subscription entity ===" ast-grep --pattern 'isDeleted() { $$$ }'Length of output: 380
Script:
#!/bin/bash # Search for subscription service and entity implementations echo "=== Searching for SubscriptionService class ===" rg -A 5 "class .*SubscriptionService" --type java echo -e "\n=== Searching for Subscription entity class ===" rg -A 5 "class .*Subscription.*implements" --type java echo -e "\n=== Searching for deletion related methods in subscription ===" rg "delete.*Subscription" --type javaLength of output: 9001
Test Results424 tests 424 ✅ 59s ⏱️ Results for commit 61a1a4d. ♻️ This comment has been updated with latest results. |
…ices - Added comprehensive unit tests for VaradhiSubscription, SubscriptionHandlers & SubscriptionService. - Refactored SubscriptionTestBase and WebTestBase for better readability and maintainability along with certain enhancements. - Enhanced tests for entity, handler, and service layers, covering all edge cases. - Ensured consistency with existing test styles and used appropriate mocking and stubbing libraries.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (1)
Line range hint
1-209: Consider adding tests for soft-deletion scenarios.While the method renaming changes are consistent and maintain existing test coverage, consider adding the following test cases to support the PR's soft-deletion feature:
- Produce attempt to a soft-deleted topic
- Produce attempt during topic restoration
- Produce after successful topic restoration
This will ensure robust validation of the new soft-deletion functionality.
🧹 Nitpick comments (30)
server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java (2)
64-67: Consider consolidating error test cases.Both test cases verify the same error condition (413 "Entity too large") with different oversized payloads (21 and 30 bytes). Consider using a parameterized test to improve test maintainability.
Example refactor:
- payload = "012345678901234567890".getBytes(); - sendRequestWithPayload( - request, payload, 413, "Entity too large.", - ErrorResponse.class - ); - - payload = "012345678901234567890123456789".getBytes(); - sendRequestWithPayload( - request, payload, 413, "Entity too large.", - ErrorResponse.class - ); + String[][] oversizedPayloads = { + {"012345678901234567890", "21 bytes"}, + {"012345678901234567890123456789", "30 bytes"} + }; + + for (String[] testCase : oversizedPayloads) { + payload = testCase[0].getBytes(); + sendRequestWithPayload( + request, payload, 413, "Entity too large.", + ErrorResponse.class + ); + }Also applies to: 70-73
Line range hint
51-74: Enhance test method documentation.While the test coverage is comprehensive, consider adding clear documentation to describe the test scenarios:
- Success cases: 10 bytes (well below limit), 19 bytes (just below limit), 20 bytes (at limit)
- Error cases: 21 bytes (just above limit), 30 bytes (well above limit)
Example improvement:
+ /** + * Validates the body handler's size limit enforcement: + * - Success cases: Tests payloads of 10, 19, and 20 bytes (limit) + * - Error cases: Tests oversized payloads of 21 and 30 bytes + * Expected: 413 "Entity too large" for payloads exceeding 20 bytes + */ @Test - public void testProduceWithForBodySize() throws InterruptedException { + public void testProduceHandlerEnforcesSizeLimit() throws InterruptedException {server/src/test/java/com/flipkart/varadhi/web/produce/HeaderValidationTest.java (2)
114-115: LGTM! Consider additional edge cases.The method name update maintains the test's validation of multi-value header counting. Consider adding test cases for:
- Empty multi-value headers
- Null values in multi-value headers
- Maximum size validation for combined multi-value header lengths
Line range hint
1-117: Consider additional test coverage for soft-deletion scenarios.Given that this PR implements soft-deletion support, consider adding test cases to verify header validation behavior for:
- Requests to soft-deleted topics
- Restoration requests
- Hard deletion requests
This would ensure header validation remains consistent across all resource states.server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (1)
399-417: Enhance error messages in property validation.While the validation logic is thorough, consider improving error messages to include:
- The invalid value that was provided
- The expected format or range
- Any additional context that would help users fix the issue
if (!validator.isValid(value, usePermissible)) { - throw new IllegalArgumentException("Invalid value for property: " + key); + throw new IllegalArgumentException(String.format( + "Invalid value '%s' for property '%s'. %s", + value, key, validator.getValidationDescription() + )); }server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (4)
46-49: Consider using more descriptive constant names.The constant names could be more descriptive to better indicate their test context. For example,
TEST_TOPIC_NAME,TEST_TEAM_NAME, etc.- private static final String TOPIC_NAME = "topic1"; - private static final String TEAM_NAME = "team1"; - private static final String ORG_NAME = "org1"; - private static final String DEFAULT_PROJECT_NAME = "project1"; + private static final String TEST_TOPIC_NAME = "topic1"; + private static final String TEST_TEAM_NAME = "team1"; + private static final String TEST_ORG_NAME = "org1"; + private static final String TEST_PROJECT_NAME = "project1";
84-111: Add documentation for route handlers.Consider adding Javadoc comments to document the purpose and behavior of each route handler. This would make it easier for other developers to understand the test setup.
+ /** + * Sets up all route handlers for topic operations including: + * - Create topic (POST /projects/:project/topics) + * - Get topic (GET /projects/:project/topics/:topic) + * - List topics (GET /projects/:project/topics) + * - Delete topic (DELETE /projects/:project/topics/:topic) + * - Restore topic (POST /projects/:project/topics/:topic/restore) + */ private void setupRoutes() {
212-229: Add verification for topic status after soft deletion.The deletion tests verify the service call but don't verify the resulting topic status. Consider adding assertions to verify the topic's state after soft deletion.
@Test void deleteTopic_WithSoftDelete_ShouldDeleteTopicSuccessfully() throws InterruptedException { + TopicResource topicResource = getTopicResource(project); + VaradhiTopic varadhiTopic = topicResource.toVaradhiTopic(); + String varadhiTopicName = String.join(".", project.getName(), TOPIC_NAME); + + doReturn(varadhiTopic).when(varadhiTopicService).get(varadhiTopicName); verifyDeleteRequest("SOFT_DELETE", ResourceDeletionType.SOFT_DELETE); + + // Verify topic status after soft deletion + TopicResource retrievedTopic = sendRequestWithoutPayload( + createRequest(HttpMethod.GET, getTopicUrl(project)), + TopicResource.class + ); + assertEquals("DELETED", retrievedTopic.getStatus()); }
261-271: Add input validation for URL construction helpers.The URL construction helpers don't validate their inputs. Consider adding null checks and validation for the project parameter.
private String getTopicsUrl(Project project) { + if (project == null || project.getName() == null) { + throw new IllegalArgumentException("Project and project name must not be null"); + } return String.join("/", "/projects", project.getName(), "topics"); } private String getTopicUrl(Project project) { + if (project == null || project.getName() == null) { + throw new IllegalArgumentException("Project and project name must not be null"); + } return String.join("/", getTopicsUrl(project), TOPIC_NAME); }server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java (2)
Line range hint
98-147: Consider adding builder pattern for test data creation.While the implementation is clean, consider introducing a builder pattern for subscription creation to make tests more maintainable and readable. This would allow for easier customization of test data without needing multiple method overloads.
Example implementation:
public static class TestSubscriptionBuilder { private String name = "test-subscription"; private Project project = PROJECT; private VaradhiTopic topic; private boolean grouped = false; private RetryPolicy retryPolicy = DEFAULT_RETRY_POLICY; // ... other fields public TestSubscriptionBuilder withName(String name) { this.name = name; return this; } // ... other builder methods public VaradhiSubscription build() { return VaradhiSubscription.of( SubscriptionResource.buildInternalName(project.getName(), name), project.getName(), topic.getName(), UUID.randomUUID().toString(), grouped, DEFAULT_ENDPOINT, retryPolicy, DEFAULT_CONSUMPTION_POLICY, DEFAULT_SHARDS, DEFAULT_SUBSCRIPTION_PROPERTIES ); } }
150-174: Consider using seeded Random for deterministic tests.Using a non-seeded Random instance could lead to non-deterministic test behavior. Consider using a seeded Random instance to ensure test reproducibility.
- private final Random random = new Random(); + private final Random random = new Random(42L); // Use a fixed seed for reproducibilityThis ensures that the random values generated in
createDlqMessageandgenerateRandomBytesare consistent across test runs, making it easier to debug test failures.server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java (4)
51-58: Good error handling in JSON deserialization!The error handling is well-implemented with proper exception wrapping. Consider adding logging before throwing the RuntimeException for better debugging capabilities.
} catch (Exception e) { + log.error("JSON deserialization failed", e); throw new RuntimeException("Failed to deserialize JSON", e); }
101-111: Good error handling for unsupported HTTP methods!Consider using a switch statement or Map-based approach for better maintainability when handling HTTP methods.
public HttpRequest<Buffer> createRequest(HttpMethod method, String path) { - if (POST == method) { - return webClient.post(DEFAULT_PORT, DEFAULT_HOST, path); - } else if (GET == method) { - return webClient.get(DEFAULT_PORT, DEFAULT_HOST, path); - } else if (DELETE == method) { - return webClient.delete(DEFAULT_PORT, DEFAULT_HOST, path); - } else if (PUT == method) { - return webClient.put(DEFAULT_PORT, DEFAULT_HOST, path); - } else { - throw new UnsupportedOperationException("Unsupported HTTP method"); - } + return switch (method) { + case POST -> webClient.post(DEFAULT_PORT, DEFAULT_HOST, path); + case GET -> webClient.get(DEFAULT_PORT, DEFAULT_HOST, path); + case DELETE -> webClient.delete(DEFAULT_PORT, DEFAULT_HOST, path); + case PUT -> webClient.put(DEFAULT_PORT, DEFAULT_HOST, path); + default -> throw new UnsupportedOperationException("Unsupported HTTP method: " + method); + }; }
168-179: Good error response handling!Consider adding validation for the error response structure to ensure all required fields are present.
ErrorResponse error = JsonMapper.jsonDeserialize(response.body().getBytes(), ErrorResponse.class); +assertNotNull(error, "Error response should not be null"); +assertNotNull(error.reason(), "Error reason should not be null"); assertEquals(expectedStatusMessage, error.reason());
220-223: Good addition of PostResponseCapture for response handling!Consider making the fields volatile since they're accessed from different threads (main thread and callback thread).
private static class PostResponseCapture<T> { - private T response; - private Throwable throwable; + private volatile T response; + private volatile Throwable throwable; }server/src/test/java/com/flipkart/varadhi/web/admin/DlqHandlersTest.java (1)
Line range hint
298-313: Consider simplifying URL building logic using URLBuilder or UriComponentsBuilder.The current string concatenation approach for building URLs with query parameters could be simplified using a dedicated URL building utility. This would:
- Reduce manual string manipulation
- Handle URL encoding automatically
- Make the code more maintainable
Example using UriComponentsBuilder:
private String getMessagesUrl(String subscriptionName, Project project, long failedAt, String nextPage, int limit) { UriComponentsBuilder builder = UriComponentsBuilder .fromPath(buildSubscriptionUrl(subscriptionName, project)) .path("/dlq/messages"); if (failedAt != 0) { builder.queryParam("earliestFailedAt", failedAt); } if (limit != -1) { builder.queryParam("limit", limit); } if (nextPage != null && !nextPage.isBlank()) { builder.queryParam("nextPage", nextPage); } return builder.build().toString(); }entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java (4)
16-36: Document magic numbers in default test constants.Consider adding comments to explain the significance of magic numbers in the default constants (e.g., 500, 1, 10). This will help other developers understand the test scenarios better.
38-56: Add assertion for initial subscription state.The successful creation test should also verify that the subscription's initial state is set correctly. Consider adding:
assertNotNull(subscription.getProperties()); +assertEquals(VaradhiSubscription.State.CREATING, subscription.getStatus().getState()); +assertNull(subscription.getStatus().getMessage());
103-116: Add test for empty properties map.Consider adding a test case for an empty properties map to ensure it's handled correctly:
@Test void createSubscription_EmptyProperties_ThrowsException() { IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, () -> { VaradhiSubscription.of( "sub1", "project1", "topic1", "description", true, DEFAULT_ENDPOINT, DEFAULT_RETRY_POLICY, DEFAULT_CONSUMPTION_POLICY, DEFAULT_SHARDS, Map.of() ); } ); assertEquals("Properties cannot be null or empty", exception.getMessage()); }
197-240: Add edge cases for integer property retrieval.Consider adding tests for:
- Integer overflow/underflow
- Empty string value
- Whitespace-only string value
Example:
@Test void getIntProperty_IntegerOverflow_ThrowsException() { VaradhiSubscription subscription = VaradhiSubscription.of( "sub1", "project1", "topic1", "description", true, DEFAULT_ENDPOINT, DEFAULT_RETRY_POLICY, DEFAULT_CONSUMPTION_POLICY, DEFAULT_SHARDS, Map.of("key", "999999999999999999999999") ); assertThrows(NumberFormatException.class, () -> subscription.getIntProperty("key")); }server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (2)
88-154: Consider refactoring the setup method for better organization.The setup method mixes infrastructure initialization with test data preparation. Consider splitting it into separate methods:
setupInfrastructure()for ZK, MetaStore, and service initializationsetupTestData()for creating test entities- Consider extracting test data values into constants
@BeforeEach void setUp() throws Exception { + setupInfrastructure(); + setupTestData(); + setupMocks(); } + +private void setupInfrastructure() throws Exception { JsonMapper.getMapper().registerSubtypes( new NamedType(PulsarStorageTopic.class, "PulsarTopic"), new NamedType(PulsarSubscription.class, "PulsarSubscription") ); zkCuratorTestingServer = new TestingServer(); zkCurator = spy(CuratorFrameworkFactory.newClient( zkCuratorTestingServer.getConnectString(), new ExponentialBackoffRetry(1000, 1) )); zkCurator.start(); varadhiMetaStore = spy(new VaradhiMetaStore(zkCurator)); orgService = new OrgService(varadhiMetaStore); teamService = new TeamService(varadhiMetaStore); meterRegistry = new JmxMeterRegistry(JmxConfig.DEFAULT, Clock.SYSTEM); projectService = new ProjectService(varadhiMetaStore, "", meterRegistry); } + +private static final String TEST_ORG_NAME = "Org"; +private static final String TEST_TEAM_NAME = "Team"; +private static final String TEST_PROJECT1_NAME = "Project1"; +private static final String TEST_PROJECT2_NAME = "Project2"; + +private void setupTestData() { org = Org.of(TEST_ORG_NAME); team = Team.of(TEST_TEAM_NAME, org.getName()); project1 = Project.of(TEST_PROJECT1_NAME, "", team.getName(), team.getOrg()); project2 = Project.of(TEST_PROJECT2_NAME, "", team.getName(), team.getOrg()); unGroupedTopic = VaradhiTopic.of("UngroupedTopic", project1.getName(), false, null); groupedTopic = VaradhiTopic.of("GroupedTopic", project2.getName(), true, null); subscription1 = createUngroupedSubscription("Sub1", project1, unGroupedTopic); subscription2 = createUngroupedSubscription("Sub2", project1, unGroupedTopic); orgService.createOrg(org); teamService.createTeam(team); projectService.createProject(project1); projectService.createProject(project2); } + +private void setupMocks() { shardProvisioner = mock(ShardProvisioner.class); doNothing().when(shardProvisioner).provision(any(), any()); controllerRestApi = mock(ControllerRestApi.class); subscriptionService = new SubscriptionService(shardProvisioner, controllerRestApi, varadhiMetaStore); }
545-631: Add test coverage for additional deletion scenarios.The deletion tests are thorough but consider adding the following test cases:
- Attempting to delete an already soft-deleted subscription
- Attempting to delete a subscription with active consumers
- Attempting to hard delete after a soft delete
Example test case:
@Test void deleteSubscription_AlreadySoftDeleted_ThrowsException() { doReturn(unGroupedTopic).when(varadhiMetaStore).getTopic(unGroupedTopic.getName()); subscriptionService.createSubscription(unGroupedTopic, subscription1, project1); // First soft delete CompletableFuture<SubscriptionState> stoppedState = CompletableFuture.completedFuture(SubscriptionState.forStopped()); doReturn(stoppedState).when(controllerRestApi) .getSubscriptionState(subscription1.getName(), REQUESTED_BY); subscriptionService.deleteSubscription( subscription1.getName(), project1, REQUESTED_BY, ResourceDeletionType.SOFT_DELETE).join(); // Attempt second delete InvalidOperationForResourceException exception = assertThrows( InvalidOperationForResourceException.class, () -> subscriptionService.deleteSubscription( subscription1.getName(), project1, REQUESTED_BY, ResourceDeletionType.SOFT_DELETE).join() ); assertEquals( "Cannot delete subscription in INACTIVE state", exception.getMessage() ); }server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (5)
63-96: Consider refactoring route configuration for better maintainability.The
configureRoutesmethod could be improved by:
- Extracting route paths into constants to avoid magic strings
- Grouping related routes (e.g., lifecycle operations) into separate methods
Example refactor:
+ private static final String BASE_PATH = "/projects/:project/subscriptions"; + private static final String SUBSCRIPTION_PATH = BASE_PATH + "/:subscription"; + private void configureRoutes() { + configureBasicOperations(); + configureLifecycleOperations(); + } + + private void configureBasicOperations() { createRoute( - HttpMethod.POST, "/projects/:project/subscriptions", + HttpMethod.POST, BASE_PATH, subscriptionHandlers::create, true ); createRoute( - HttpMethod.GET, "/projects/:project/subscriptions/:subscription", + HttpMethod.GET, SUBSCRIPTION_PATH, subscriptionHandlers::get, false ); // ... other basic operations + } + + private void configureLifecycleOperations() { createRoute( - HttpMethod.POST, "/projects/:project/subscriptions/:subscription/restore", + HttpMethod.POST, SUBSCRIPTION_PATH + "/restore", subscriptionHandlers::restore, false ); // ... other lifecycle operations }
141-143: Enhance error response validation in tests.Consider extending the error response validation to include:
- HTTP response headers verification
- Error message format consistency checks
- Error code validation where applicable
Example enhancement for one of the error cases:
ErrorResponse response = sendRequestWithEntity(request, resource, 401, errorMessage, ErrorResponse.class); + // Verify error response format assertEquals(errorMessage, response.reason()); + assertEquals("UNAUTHORIZED", response.code()); + verify(response.headers().contains("X-Error-Code"));Also applies to: 155-157, 169-171, 184-186, 197-199, 209-211, 221-223
258-306: Add tests for concurrent deletion scenarios and resource cleanup.The deletion tests should be extended to cover:
- Concurrent deletion attempts
- Verification of related resource cleanup
- Rollback scenarios when deletion partially fails
Example additional test:
@Test void deleteSubscription_ConcurrentDeletion_HandlesGracefully() throws InterruptedException { HttpRequest<Buffer> request1 = createRequest( HttpMethod.DELETE, buildSubscriptionUrl("sub1", PROJECT) + "?deletionType=SOFT_DELETE" ); HttpRequest<Buffer> request2 = createRequest( HttpMethod.DELETE, buildSubscriptionUrl("sub1", PROJECT) + "?deletionType=SOFT_DELETE" ); doReturn(PROJECT).when(projectService).getCachedProject(PROJECT.getName()); doReturn(CompletableFuture.completedFuture(null)) .doThrow(new ConcurrentModificationException("Resource already being deleted")) .when(subscriptionService) .deleteSubscription(anyString(), eq(PROJECT), any(), eq(ResourceDeletionType.SOFT_DELETE)); sendRequestWithoutPayload(request1, null); ErrorResponse response = sendRequestWithoutPayload(request2, 409, "Resource already being deleted", ErrorResponse.class); assertEquals("Resource already being deleted", response.reason()); }
328-350: Add tests for version conflicts and partial updates.The update tests should be extended to cover:
- Version conflict scenarios
- Partial update capabilities
- Update retries with version checks
Example additional test:
@Test void updateSubscription_VersionConflict_ThrowsConflictException() throws InterruptedException { HttpRequest<Buffer> request = createRequest(HttpMethod.PUT, buildSubscriptionUrl("sub1", PROJECT)); SubscriptionResource resource = createSubscriptionResource("sub1", PROJECT, TOPIC_RESOURCE); when(subscriptionService.updateSubscription( any(), anyInt(), anyString(), anyBoolean(), any(), any(), any(), any() )).thenThrow(new ConflictException("Version conflict")); ErrorResponse response = sendRequestWithEntity( request, resource, 409, "Version conflict", ErrorResponse.class); assertEquals("Version conflict", response.reason()); }
Line range hint
43-472: Consider overall test improvements for better maintainability.While the test coverage is comprehensive, consider the following improvements:
- Add JavaDoc comments describing test scenarios and edge cases
- Implement test data builders to reduce duplication and improve maintainability
- Group related tests using nested test classes (JUnit 5 @nested)
Example implementation:
@Builder class SubscriptionTestData { private String name; private Project project; private TopicResource topic; private RetryPolicy retryPolicy; public static SubscriptionTestData defaultData() { return builder() .name("sub1") .project(Project.of("project1", "", "team1", "org1")) .topic(TopicResource.builder().build()) .retryPolicy(RetryPolicy.builder().build()) .build(); } } @Nested class SubscriptionCreationTests { @Test void whenValidInput_thenCreatesSuccessfully() { // Test implementation using SubscriptionTestData } }server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (1)
125-125: Consider extending test coverage for soft-deleted state.While the method replacement is correct, given the PR's objective to add soft-deletion support, consider adding test cases for the soft-deleted state alongside the existing Blocked, Throttled, and Replicating states.
server/src/test/java/com/flipkart/varadhi/services/DlqServiceTest.java (2)
182-183: Enhance helper method to support deletion statesThe
setupSubscriptionForGetMessageshelper method should be enhanced to:
- Support creation of soft-deleted subscriptions
- Allow specification of subscription state for better test coverage
-private VaradhiSubscription setupSubscriptionForGetMessages() { +private VaradhiSubscription setupSubscriptionForGetMessages(boolean isDeleted) { String consumerId = "consumerId"; VaradhiTopic vTopic = TOPIC_RESOURCE.toVaradhiTopic(); VaradhiSubscription subscription = spy(createUngroupedSubscription("sub12", PROJECT, vTopic)); + when(subscription.isDeleted()).thenReturn(isDeleted); // ... rest of the method return subscription; }
Line range hint
1-196: Enhance test coverage for soft-deletion functionalityWhile the code changes maintain the existing test functionality, the test suite needs to be expanded to thoroughly cover the new soft-deletion feature. Consider adding the following test cases:
- Lifecycle tests for soft-deleted subscriptions
- DLQ operations on restored subscriptions
- Error handling for various deletion states
This will ensure the robustness of the new deletion functionality and prevent potential issues in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java(3 hunks)server/src/test/java/com/flipkart/varadhi/services/DlqServiceTest.java(5 hunks)server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java(5 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/DlqHandlersTest.java(8 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/OrgHandlersTest.java(4 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/ProjectHandlersTest.java(5 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java(3 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/TeamHandlersTest.java(3 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/authz/IamPolicyHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/HeaderValidationTest.java(5 hunks)server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/src/test/java/com/flipkart/varadhi/web/authz/IamPolicyHandlersTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (31)
server/src/test/java/com/flipkart/varadhi/web/produce/BodyHandlerTest.java (3)
52-53: LGTM! Good test coverage for normal payload size.Testing with a 10-byte payload (well within the 20-byte limit) verifies basic successful case.
56-57: LGTM! Good edge case testing.Testing with a 19-byte payload verifies behavior just under the limit.
60-61: LGTM! Excellent boundary testing.Testing with exactly 20 bytes (at the limit) verifies the boundary condition.
server/src/test/java/com/flipkart/varadhi/web/produce/HeaderValidationTest.java (4)
65-65: LGTM! Method name updated consistently.The change from
sendRequestWithByteBufferBodytosendRequestWithPayloadmaintains the test's functionality while using the new method name.Also applies to: 69-69
Line range hint
78-82: LGTM! Error validation maintained.The method name update preserves the test's ability to verify header size constraints and error responses.
Line range hint
89-93: LGTM! Value size validation preserved.The method name update maintains the test's validation of header value size constraints.
Line range hint
102-106: LGTM! Header count validation maintained.The method name update preserves the test's ability to verify the maximum header count constraint.
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (4)
36-39: Well-structured class-level changes!Good improvements in code organization:
- Comprehensive class-level Javadoc
- Making
NUMBER_OF_RETRIES_ALLOWEDstatic final improves immutabilityAlso applies to: 44-44
230-231: Consider addressing the TODO comments before merging.The comments indicate potential improvements:
- Splitting into separate update APIs
- Adding validation for unchanged updates
Would you like me to help create separate issues to track these improvements?
116-120: LGTM: Well-implemented restoration endpoint!The restoration endpoint is properly:
- Authorized with SUBSCRIPTION_UPDATE permission
- Implemented as a non-blocking operation
- Returns the restored subscription resource
Also applies to: 271-273
333-336: LGTM: Proper security check for ignoreConstraints.Good implementation of the super admin check for ignoring constraints. This ensures that only authorized users can bypass validation rules.
server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java (5)
4-15: LGTM! Well-organized imports.The new imports are properly organized and necessary for the enhanced functionality.
40-82: Well-structured constants and field declarations!Good improvements:
- Clear naming with DEFAULT_ prefix
- Descriptive variable names (e.g., 'random' instead of 'r')
- Proper organization of default configurations
- Clean mock declarations using annotations
84-88: LGTM! Proper mock initialization.The setup follows testing best practices with proper parent class initialization and mock setup.
90-96: Good method renaming from 'get' to 'build'!The rename better reflects that these methods construct URLs rather than retrieve resources.
177-183: LGTM! Well-structured retry policy creation.The custom retry policy creation method is clean and follows the pattern of other factory methods in the class.
server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java (3)
47-49: Well-structured constant definitions!Good practice to extract magic numbers into named constants, improving code maintainability and readability.
84-87: Good addition of assertion message!The descriptive assertion message will help in debugging test failures.
114-159: Excellent improvements to request handling methods!
- Clear method naming distinguishing between payload and entity
- Good use of Optional for null handling
- Helpful assertion messages for debugging
server/src/test/java/com/flipkart/varadhi/web/admin/OrgHandlersTest.java (1)
67-67: LGTM! Method renaming improves clarity.The renaming of request handling methods enhances readability and better aligns with REST terminology:
sendRequestWithBody→sendRequestWithEntitysendRequestWithoutBody→sendRequestWithoutPayloadAlso applies to: 73-73, 78-78, 98-98, 109-109, 115-115, 141-141, 146-146, 150-150
server/src/test/java/com/flipkart/varadhi/web/admin/ProjectHandlersTest.java (1)
83-83: LGTM! Consistent method renaming across test files.The changes maintain consistency with the renaming pattern established in
OrgHandlersTest.java.Also applies to: 89-89, 95-95, 100-100, 112-112, 118-118, 127-127, 132-132, 141-141, 151-151, 157-157, 163-163
server/src/test/java/com/flipkart/varadhi/web/admin/TeamHandlersTest.java (1)
85-85: LGTM! Method renaming maintains consistency.The changes follow the same renaming pattern seen in other test files, improving overall codebase consistency.
Also applies to: 91-91, 97-97, 102-102, 114-114, 121-121
server/src/test/java/com/flipkart/varadhi/web/admin/DlqHandlersTest.java (1)
70-73: LGTM! Method renaming improves clarity of intent.The renaming changes better reflect the methods' purpose:
getSubscriptionResource→createSubscriptionResourcegetUngroupedSubscription→createUngroupedSubscriptionentities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java (1)
1-241: Well-structured and comprehensive test suite.The test suite is well-organized and provides good coverage of the
VaradhiSubscriptionclass's functionality, particularly the state management features needed for soft deletion. The tests are focused, readable, and follow good testing practices.server/src/test/java/com/flipkart/varadhi/web/produce/ProduceHandlersTest.java (6)
102-102: LGTM! Method replacement maintains error handling test.The change preserves the test's ability to verify ResourceNotFoundException handling.
143-147: LGTM! Well-formatted error handling test.The method replacement maintains proper error handling verification with clear formatting.
157-161: LGTM! Comprehensive error handling test coverage.The changes maintain proper verification of both expected (ResourceNotFoundException) and unexpected (RuntimeException) error scenarios.
171-171: LGTM! ProduceException handling test maintained.The change preserves proper verification of ProduceException scenarios.
Line range hint
189-208: LGTM! Header ordering and project validation tests maintained.The changes preserve proper verification of header ordering and non-existing project scenarios.
Line range hint
77-89: LGTM! Verify the availability of sendRequestWithPayload method.The method renaming improves clarity while maintaining the test's functionality for message production and duplication scenarios.
✅ Verification successful
Verified: sendRequestWithPayload method is properly implemented and available
The method is correctly implemented in WebTestBase.java and properly handles payload conversion, status verification, and response deserialization. The usage in ProduceHandlersTest.java is consistent with its implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and implementation of sendRequestWithPayload ast-grep --pattern 'sendRequestWithPayload($_, $_, $_)'Length of output: 1754
Script:
#!/bin/bash # Check the implementation of sendRequestWithPayload in WebTestBase ast-grep --pattern 'sendRequestWithPayload($_, $_, $_) { $$$ }'Length of output: 67
Script:
#!/bin/bash # Search for sendRequestWithPayload definition with context rg -U "sendRequestWithPayload.*?\{" -A 10 server/src/test/java/com/flipkart/varadhi/web/WebTestBase.javaLength of output: 106
Script:
#!/bin/bash # View the entire WebTestBase.java file cat server/src/test/java/com/flipkart/varadhi/web/WebTestBase.javaLength of output: 9205
server/src/test/java/com/flipkart/varadhi/services/DlqServiceTest.java (1)
83-83: Add test coverage for soft-deleted subscriptionsThe test should verify DLQ message retrieval behavior when:
- The subscription is soft-deleted
- The subscription is restored from soft-deletion
server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java
Outdated
Show resolved
Hide resolved
entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (2)
251-254: Consider using a constant for the default deletion type.The default deletion type could be extracted to a constant to improve maintainability and make the default behavior more explicit.
+ private static final ResourceDeletionType DEFAULT_DELETION_TYPE = ResourceDeletionType.SOFT_DELETE; ResourceDeletionType deletionType = ctx.queryParam(QUERY_PARAM_DELETION_TYPE).stream() .map(ResourceDeletionType::fromValue) .findFirst() - .orElse(ResourceDeletionType.SOFT_DELETE); + .orElse(DEFAULT_DELETION_TYPE);
413-418: Enhance property validation error handling.Consider these improvements to the property validation:
- Provide more descriptive error messages including the invalid value
- Collect all validation errors before throwing the exception
+ List<String> validationErrors = new ArrayList<>(); propertyValidators.forEach((key, validator) -> { String value = properties.get(key); if (!validator.isValid(value, usePermissible)) { - throw new IllegalArgumentException("Invalid value for property: " + key); + validationErrors.add(String.format("Invalid value '%s' for property: %s", value, key)); } }); + if (!validationErrors.isEmpty()) { + throw new IllegalArgumentException("Validation failed:\n" + String.join("\n", validationErrors)); + }server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (2)
63-107: Consider extracting route configuration to the base class.The
configureRoutesmethod and its helpercreateRoutecontain common route setup logic that could be reused by other handler tests.Consider moving this to
SubscriptionTestBase:- private void configureRoutes() { + protected void configureRoutes() {- private void createRoute(HttpMethod method, String path, Handler<RoutingContext> handler, boolean requiresBody) { + protected void createRoute(HttpMethod method, String path, Handler<RoutingContext> handler, boolean requiresBody) {
338-344: Consider using ArgumentMatchers for better readability.The test uses a mix of argument captors and matchers. Consider using specific argument matchers for better readability.
- when(subscriptionService.updateSubscription( - stringCaptor.capture(), integerCaptor.capture(), anyString(), anyBoolean(), any(), any(), any(), - any() - )).thenReturn( + when(subscriptionService.updateSubscription( + eq("project1.sub1"), eq(0), anyString(), anyBoolean(), any(), any(), any(), + any() + )).thenReturn(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java(3 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-workflow-docker-build / build
- GitHub Check: build
🔇 Additional comments (7)
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (3)
43-44: LGTM! Good improvements to initialization and constants.The changes improve code quality through:
- Making NUMBER_OF_RETRIES_ALLOWED a proper constant with static final
- Cleaner initialization of validators
- Better documentation with comprehensive Javadoc
Also applies to: 73-75
116-120: LGTM! Well-structured restore endpoint.The new restore endpoint is properly implemented with:
- Appropriate authorization (SUBSCRIPTION_UPDATE)
- RESTful convention (/restore)
- Non-blocking execution
271-274: LGTM! Clean implementation of restore functionality.The restore method follows the established patterns:
- Proper error handling through handleResponse
- Consistent with other endpoint implementations
server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (4)
47-51: LGTM! Good use of argument captors.The use of
@Captorfor string and integer arguments will help in verifying the exact values passed to the service methods.
257-306: Comprehensive test coverage for deletion types.Good test coverage for soft deletion scenarios including:
- Explicit soft delete
- Default to soft delete when no type is specified
- Proper verification of service method calls
427-468: Enhance lifecycle operation tests.While the basic lifecycle operation tests are present, they could be enhanced to cover more scenarios.
Consider adding tests for:
- Error scenarios (e.g., restoring an active subscription)
- Invalid state transitions
- Concurrent operation handling
Example:
@Test void restoreSubscription_AlreadyActive_ThrowsIllegalStateException() throws InterruptedException { HttpRequest<Buffer> request = createRequest( HttpMethod.POST, buildSubscriptionUrl("sub1", PROJECT) + "/restore" ); doReturn(CompletableFuture.failedFuture( new IllegalStateException("Subscription is already active") )).when(subscriptionService).restoreSubscription(any(), any()); ErrorResponse response = sendRequestWithoutPayload( request, 400, "Subscription is already active", ErrorResponse.class); assertEquals("Subscription is already active", response.reason()); }
189-200: Add test for maximum retry limit configuration.The test hardcodes the maximum retry limit to 3. Consider parameterizing this value based on configuration.
…lidation - Added message as an input query parameter in delete and restore requests (not mandatory). - Introduced Action Code as an enum to track who performed the action (User, Varadhi Admin, System). - Implemented logic to allow actions based on who performed the previous action. - Updated validation logic for topic deletion: - Soft deletion of a topic is allowed only if all subscriptions are soft deleted. - Hard deletion of a topic is allowed only if all subscriptions are hard deleted. - Added `includeInactive` flag to resource listing to include inactive resources. - Added unit tests to verify the new functionality. - Added a generic `LifecycleStatus` class to handle both topic and subscription lifecycle statuses. - Refactored existing code to use the new `LifecycleStatus` class.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (1)
104-118: 🛠️ Refactor suggestionImplement rollback mechanism for hard delete failures.
If storage topic deletion fails for some regions, the system could be left in an inconsistent state.
Consider implementing a two-phase deletion:
- Mark all storage topics for deletion
- Execute deletion with rollback capability
server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (1)
338-355:⚠️ Potential issueImplement retry mechanism for hard deletion.
The hard deletion implementation could leave the subscription in an inconsistent state if
deProvisionfails after updating the status but before completing deletion.Consider implementing a retry mechanism or transaction to ensure atomicity:
private void handleHardDelete( VaradhiSubscription subscription, Project subProject, ResourceActionRequest actionRequest ) { subscription.markDeleting(actionRequest.actionCode(), actionRequest.message()); metaStore.updateSubscription(subscription); + int maxRetries = 3; + int retryCount = 0; try { - shardProvisioner.deProvision(subscription, subProject); - metaStore.deleteSubscription(subscription.getName()); - log.info("Subscription '{}' deleted successfully.", subscription.getName()); + while (retryCount < maxRetries) { + try { + shardProvisioner.deProvision(subscription, subProject); + metaStore.deleteSubscription(subscription.getName()); + log.info("Subscription '{}' deleted successfully.", subscription.getName()); + return; + } catch (Exception e) { + retryCount++; + if (retryCount == maxRetries) throw e; + log.warn("Retry {} of {} for deleting subscription '{}'", + retryCount, maxRetries, subscription.getName(), e); + Thread.sleep(1000 * retryCount); + } + } } catch (Exception e) { log.error("Failed to hard delete subscription '{}'.", subscription.getName(), e); subscription.markDeleteFailed(e.getMessage()); metaStore.updateSubscription(subscription); throw e; } }
🧹 Nitpick comments (26)
server/src/main/java/com/flipkart/varadhi/web/routes/RouteDefinition.java (1)
71-74: LGTM! Consider adding method-level documentation.The implementation correctly adds PATCH support following the existing pattern. This aligns well with the PR objective of implementing soft deletion functionality, as PATCH is the appropriate HTTP method for partial updates like marking resources as deleted.
Consider adding method-level documentation to explain the purpose and usage, similar to other HTTP method builders:
+ /** + * Creates a new Builder instance for PATCH requests. + * @param name The name of the route + * @param path The path pattern for the route + * @return A new Builder instance configured for PATCH method + */ public static Builder patch(String name, String path) { return new Builder(name, HttpMethod.PATCH, path); }server/src/test/java/com/flipkart/varadhi/web/entities/TopicResourceTest.java (1)
17-77: Add test coverage for edge cases and error conditions.While the current tests cover the basic functionality well, consider adding tests for:
Edge cases:
- Topics with special characters in names
- Maximum length names
- Empty or null project names
Error conditions:
- Invalid action codes
- Null capacity policies
- Invalid conversions
Negative scenarios:
- Validation failures
- Conversion failures
Here's a sample test for validation failures:
@Test void from_ThrowsException_WhenInvalidTopicName() { assertThrows(IllegalArgumentException.class, () -> { VaradhiTopic varadhiTopic = VaradhiTopic.of( "projectName", "invalid/topic/name", true, new TopicCapacityPolicy(), LifecycleStatus.ActionCode.SYSTEM_ACTION ); TopicResource.from(varadhiTopic); }); }server/src/main/java/com/flipkart/varadhi/utils/VaradhiTopicFactory.java (1)
22-27: Consider creating a tracking issue for the TODO.The comment indicates that the
deploymentRegionfield is a temporary solution. To ensure this technical debt is properly tracked and addressed:
- The region should be derived from TopicResource
- Regional/HA/BCP-DR policies need to be implemented
Would you like me to help create an issue to track this technical debt?
server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (2)
88-115: Consider breaking down the route setup method for better maintainability.The
setupRoutesmethod handles multiple responsibilities. Consider extracting each route setup into separate methods for better organization and readability.- private void setupRoutes() { + private void setupRoutes() { + setupCreateTopicRoute(); + setupGetTopicRoute(); + setupListTopicsRoute(); + setupDeleteTopicRoute(); + setupRestoreTopicRoute(); + setupFailureHandlers(); + } + + private void setupCreateTopicRoute() { router.post("/projects/:project/topics") .handler(bodyHandler) .handler(ctx -> { topicHandlers.setRequestBody(ctx); ctx.next(); }) .handler(ctx -> { requestTelemetryConfigurator.addRequestSpanAndLog(ctx, "CreateTopic", TelemetryType.ALL); ctx.next(); }) .handler(wrapBlocking(topicHandlers::create)); + }
229-247: Consider adding edge cases for deletion type validation.While the deletion type tests are comprehensive, consider adding test cases for:
- Empty string deletion type
- Case-insensitive deletion type handling
@Test void deleteTopic_WithEmptyDeletionType_ShouldDefaultToDefault() throws InterruptedException { verifyDeleteRequest("", ResourceDeletionType.DEFAULT); } @Test void deleteTopic_WithCaseInsensitiveDeletionType_ShouldWork() throws InterruptedException { verifyDeleteRequest("soft_delete", ResourceDeletionType.SOFT_DELETE); }server/src/test/java/com/flipkart/varadhi/utils/VaradhiTopicFactoryTest.java (1)
90-112: Consider alternatives to testing private methods with reflection.While the test is well-implemented, using reflection to test private methods is generally not recommended as it:
- Makes tests brittle and harder to maintain
- May indicate that the private method should be extracted into a separate class
Consider either:
- Moving the deployment logic to a separate strategy class
- Making the method package-private for testing
- Testing the behavior through public methods instead
server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java (1)
48-55: Enhance error message in JSON deserialization.While the error handling is good, consider including more context in the error message.
- throw new RuntimeException("Failed to deserialize JSON", e); + throw new RuntimeException( + String.format("Failed to deserialize JSON to %s<%s>", collectionClass.getSimpleName(), clazz.getSimpleName()), + e + );server/src/test/java/com/flipkart/varadhi/web/entities/SubscriptionResourceTest.java (2)
17-35: Add test cases for edge cases and invalid inputs.While the happy path is well tested, consider adding test cases for:
- Invalid subscription names (null, empty, special characters)
- Null project or topic
- Edge cases for other properties
This will ensure the validation logic is working correctly.
61-73: Add test cases for internal name edge cases.Consider adding test cases for:
- Project names with special characters
- Subscription names with special characters
- Empty or null project/subscription names
- Maximum length validation
This will ensure the internal name generation is robust against all inputs.
entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (2)
43-61: Consider adding parameter validation.While the factory method changes are good, consider adding validation for the
actionCodeparameter to prevent null values.public static VaradhiTopic of( String project, String name, boolean grouped, TopicCapacityPolicy capacity, LifecycleStatus.ActionCode actionCode ) { + if (actionCode == null) { + throw new IllegalArgumentException("Action code cannot be null"); + } return new VaradhiTopic( buildTopicName(project, name), INITIAL_VERSION, grouped, capacity, new HashMap<>(), new LifecycleStatus(LifecycleStatus.State.ACTIVE, actionCode) ); }
107-135: Consider adding parameter validation and thread safety.The lifecycle management methods look good but consider:
- Adding null checks for parameters in
markActiveandmarkInactive.- Adding synchronization or making the methods thread-safe if concurrent modifications are possible.
public void markActive(LifecycleStatus.ActionCode actionCode, String message) { + if (actionCode == null) { + throw new IllegalArgumentException("Action code cannot be null"); + } + if (message == null) { + throw new IllegalArgumentException("Message cannot be null"); + } this.status.update(LifecycleStatus.State.ACTIVE, message, actionCode); } public void markInactive(LifecycleStatus.ActionCode actionCode, String message) { + if (actionCode == null) { + throw new IllegalArgumentException("Action code cannot be null"); + } + if (message == null) { + throw new IllegalArgumentException("Message cannot be null"); + } this.status.update(LifecycleStatus.State.INACTIVE, message, actionCode); }If concurrent modifications are possible, consider adding synchronization:
+import java.util.concurrent.locks.ReentrantReadWriteLock; public class VaradhiTopic extends AbstractTopic { + private final ReentrantReadWriteLock statusLock = new ReentrantReadWriteLock(); public void markActive(LifecycleStatus.ActionCode actionCode, String message) { + statusLock.writeLock().lock(); + try { this.status.update(LifecycleStatus.State.ACTIVE, message, actionCode); + } finally { + statusLock.writeLock().unlock(); + } } public void markInactive(LifecycleStatus.ActionCode actionCode, String message) { + statusLock.writeLock().lock(); + try { this.status.update(LifecycleStatus.State.INACTIVE, message, actionCode); + } finally { + statusLock.writeLock().unlock(); + } } @JsonIgnore public boolean isActive() { + statusLock.readLock().lock(); + try { return this.status.getState() == LifecycleStatus.State.ACTIVE; + } finally { + statusLock.readLock().unlock(); + } } }server/src/test/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactoryTest.java (2)
25-40: Consider moving test constants to a shared test constants file.The test constants could be reused across other test classes. Consider extracting them to a shared test constants file to promote reusability and maintainability.
41-47: Consider verifying mock initialization.While the setup is correct, consider adding assertions to verify that the mocks are properly initialized and injected. This can help catch initialization issues early.
Example:
@Override @BeforeEach public void setUp() { MockitoAnnotations.openMocks(this); varadhiSubscriptionFactory = new VaradhiSubscriptionFactory(topicService, subscriptionFactory, topicFactory, REGION); + assertNotNull(subscriptionFactory, "subscriptionFactory should be initialized"); + assertNotNull(topicFactory, "topicFactory should be initialized"); + assertNotNull(topicService, "topicService should be initialized"); }entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java (2)
19-31: Consider adding Javadoc to document the helper class and method.The helper class and method effectively reduce code duplication. Consider adding Javadoc to explain their purpose and parameters.
+/** + * A dummy implementation of StorageTopic for testing purposes. + */ @EqualsAndHashCode(callSuper = true) public static class DummyStorageTopic extends StorageTopic { +/** + * Creates a VaradhiTopic instance with default parameters. + * @param grouped whether the topic is grouped + * @return a new VaradhiTopic instance + */ private VaradhiTopic createDefaultVaradhiTopic(boolean grouped) {
111-146: Consider adding tests for invalid state transitions.The tests cover basic state transitions well. Consider adding tests for:
- Invalid state transitions (e.g., attempting to activate an already active topic)
- Invalid action codes
- Empty or null reason messages
+@Test +void markActive_WhenAlreadyActive_ThrowsException() { + VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false); + + assertThrows(IllegalStateException.class, () -> + varadhiTopic.markActive(LifecycleStatus.ActionCode.SYSTEM_ACTION, "Already active")); +} +@Test +void markInactive_WithNullReason_ThrowsException() { + VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false); + + assertThrows(IllegalArgumentException.class, () -> + varadhiTopic.markInactive(LifecycleStatus.ActionCode.SYSTEM_ACTION, null)); +}server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (3)
55-70: Enhance error message for inactive topics.The error message could be more descriptive by indicating that the topic exists but is inactive.
- throw new ResourceNotFoundException("Topic %s not found.".formatted(topicName)); + throw new ResourceNotFoundException("Topic %s exists but is inactive.".formatted(topicName));
81-81: Track TODO for namespace cleanup.The TODO comment about namespace cleanup should be tracked in the issue tracker.
Would you like me to create an issue to track the namespace cleanup implementation?
203-206: Consider pushing filter to database layer.The current implementation loads all topics and filters in memory. Consider adding a filter parameter to the MetaStore query.
entities/src/main/java/com/flipkart/varadhi/entities/auth/ResourceAction.java (1)
91-118: Consider tightening validation or naming for the nestedActionenum
The addition of multiple actions (RESTORE, CONSUME, PRODUCE, etc.) is comprehensive. However, if certain actions require special handling or mandatory parameters at runtime, consider adding validations or distinct methods to prevent misuse. For example, some consumers might attempt toRESTOREorMIGRATEresources without the correct prerequisites.server/src/main/java/com/flipkart/varadhi/web/entities/TopicResource.java (1)
22-32: Check field mutability versus final usage
Fieldscapacity(line 26) andactionCode(line 31) are assigned in the constructor but also have@Setter. Confirm if it’s necessary for these fields to be mutable after construction; if not, consider removing the setters to maintain immutability and reduce accidental state changes.server/src/main/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactory.java (1)
157-196: Improve shard capacity and multi-shard logic
ThegetMultiShardandgetShardCapacitymethods (lines 157–196) cleanly separate single vs. multiple shard creation. However, if advanced load balancing or dynamic scaling is introduced in the future, you may want to expand these methods to factor in usage metrics or cluster constraints beyond a simple1/shardCountcapacity split.entities/src/main/java/com/flipkart/varadhi/entities/ResourceActionRequest.java (1)
14-18: Consider enhancing the error message.The error message could be more descriptive to help with debugging.
- throw new NullPointerException("actionCode must not be null"); + throw new NullPointerException("ResourceActionRequest: actionCode must not be null");entities/src/test/java/com/flipkart/varadhi/entities/ResourceActionRequestTest.java (1)
10-59: Consider adding tests for Validatable interface.Since
ResourceActionRequestimplementsValidatable, consider adding tests to verify the validation behavior.@Test void validate_ValidatesSuccessfully() { ResourceActionRequest request = new ResourceActionRequest( LifecycleStatus.ActionCode.SYSTEM_ACTION, "Test message" ); request.validate(); // Should not throw any exception }entities/src/test/java/com/flipkart/varadhi/entities/LifecycleStatusTest.java (1)
10-68: Consider adding tests for invalid state transitions.Since this is related to lifecycle management, it would be valuable to add tests that verify the behavior when invalid state transitions are attempted.
@Test void update_InvalidStateTransition_ThrowsException() { LifecycleStatus status = new LifecycleStatus( LifecycleStatus.State.DELETED, LifecycleStatus.ActionCode.SYSTEM_ACTION ); assertThrows(IllegalStateException.class, () -> status.update(LifecycleStatus.State.CREATING, "Invalid transition") ); }entities/src/testFixtures/java/com/flipkart/varadhi/entities/SubscriptionUtils.java (1)
56-59: Consider adding validation for negative test scenarios.The subscription creation with
LifecycleStatus.ActionCode.SYSTEM_ACTIONlooks good, but consider adding utility methods for creating subscriptions with different lifecycle states to support negative test cases.+ /** + * Creates a subscription with a specific lifecycle status for testing. + * + * @param name the subscription name + * @param project the project name + * @param description the description + * @param actionCode the lifecycle status action code + * @return the subscription + */ + public static VaradhiSubscription createWithStatus( + String name, + String project, + String description, + LifecycleStatus.ActionCode actionCode + ) { + return TopicResource.unGrouped(name, project, description, actionCode); + }server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java (1)
183-197: Consider adding transaction boundaries for deletion operations.The deletion logic looks good, but there's a potential race condition between state check and actual deletion. Consider adding transaction boundaries or optimistic locking.
Also, enhance logging to include the deletion type for better traceability:
- log.info("Subscription '{}' deleted successfully.", subscription.getName()); + log.info("Subscription '{}' {} successfully.", subscription.getName(), + deletionType.equals(ResourceDeletionType.HARD_DELETE) ? "hard deleted" : "soft deleted");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
common/src/main/java/com/flipkart/varadhi/Constants.java(3 hunks)controller/src/test/java/com/flipkart/varadhi/controller/AssignmentManagerTest.java(6 hunks)controller/src/test/java/com/flipkart/varadhi/controller/ControllerApiMgrTest.java(22 hunks)controller/src/test/java/com/flipkart/varadhi/controller/OperationMgrTest.java(17 hunks)controller/src/test/java/com/flipkart/varadhi/controller/impl/LeastAssignedStrategyTests.java(5 hunks)entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/ResourceActionRequest.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/VaradhiSubscription.java(4 hunks)entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/auth/ResourceAction.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/LifecycleStatusTest.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/ResourceActionRequestTest.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/auth/ResourceActionTest.java(1 hunks)entities/src/testFixtures/java/com/flipkart/varadhi/entities/SubscriptionUtils.java(5 hunks)producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java(3 hunks)pulsar/src/test/java/com/flipkart/varadhi/pulsar/PulsarProducerFactoryTest.java(1 hunks)pulsar/src/test/java/com/flipkart/varadhi/pulsar/PulsarStackProviderTest.java(1 hunks)pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarProducerTest.java(1 hunks)pulsar/src/test/java/com/flipkart/varadhi/pulsar/services/PulsarTopicServiceTest.java(4 hunks)pulsar/src/testE2E/java/com/flipkart/varadhi/pulsar/PulsarTopicServiceTest.java(2 hunks)server/src/main/java/com/flipkart/varadhi/config/RestOptions.java(1 hunks)server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java(2 hunks)server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java(1 hunks)server/src/main/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactory.java(12 hunks)server/src/main/java/com/flipkart/varadhi/utils/VaradhiTopicFactory.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/entities/SubscriptionResource.java(5 hunks)server/src/main/java/com/flipkart/varadhi/web/entities/TopicResource.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/routes/RouteDefinition.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java(3 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java(4 hunks)server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactoryTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/utils/VaradhiTopicFactoryTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java(5 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/entities/SubscriptionResourceTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/entities/TopicResourceTest.java(1 hunks)server/src/testE2E/java/com/flipkart/varadhi/AuthZProviderTests.java(3 hunks)server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java(8 hunks)server/src/testE2E/java/com/flipkart/varadhi/OrgTests.java(1 hunks)server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java(10 hunks)server/src/testE2E/java/com/flipkart/varadhi/TopicTests.java(5 hunks)
✅ Files skipped from review due to trivial changes (5)
- pulsar/src/test/java/com/flipkart/varadhi/pulsar/entities/PulsarProducerTest.java
- controller/src/test/java/com/flipkart/varadhi/controller/AssignmentManagerTest.java
- controller/src/test/java/com/flipkart/varadhi/controller/OperationMgrTest.java
- controller/src/test/java/com/flipkart/varadhi/controller/ControllerApiMgrTest.java
- pulsar/src/testE2E/java/com/flipkart/varadhi/pulsar/PulsarTopicServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java
- server/src/main/java/com/flipkart/varadhi/config/RestOptions.java
- server/src/testE2E/java/com/flipkart/varadhi/TopicTests.java
- server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java
- producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java
- common/src/main/java/com/flipkart/varadhi/Constants.java
- entities/src/main/java/com/flipkart/varadhi/entities/VaradhiSubscription.java
- server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (60)
pulsar/src/test/java/com/flipkart/varadhi/pulsar/PulsarProducerFactoryTest.java (1)
35-35: LGTM! Constant naming convention update.The change from
DefaultTopicCapacitytoDEFAULT_TOPIC_CAPACITYaligns with Java's standard naming convention for constants (uppercase with underscores).server/src/test/java/com/flipkart/varadhi/web/entities/TopicResourceTest.java (2)
1-14: LGTM! Clean package structure and imports.The package structure follows Java conventions, and imports are well-organized with appropriate static imports for JUnit assertions.
15-15: LGTM! Test class follows naming conventions.server/src/main/java/com/flipkart/varadhi/utils/VaradhiTopicFactory.java (3)
3-8: LGTM!The imports are well-organized and all are being utilized in the code.
Also applies to: 12-12
30-44: LGTM!The constructor documentation and implementation are well-structured and clear.
65-77: LGTM!The
planDeploymentmethod is well-documented and properly implements the topic deployment logic.server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (2)
250-257: Add negative test cases for restore operation.The restore operation only tests the happy path. Consider adding test cases for:
- Restoring a non-existent topic
- Restoring a topic that wasn't soft-deleted
- Restoring a hard-deleted topic
274-291: LGTM! Helper methods are well-structured.The helper methods are focused, maintainable, and follow good practices:
- Clear error response assertion
- Consistent URL construction
- Reusable topic resource creation
controller/src/test/java/com/flipkart/varadhi/controller/impl/LeastAssignedStrategyTests.java (5)
26-27: LGTM! Builder pattern refactoring looks good.The change from
getBuilder()tobuilder()follows a cleaner builder pattern implementation while maintaining the test's functionality.
36-36: LGTM! Builder usage is consistent.The builder pattern change is applied consistently while preserving the test's comprehensive verification of shard assignments and resource management.
55-55: LGTM! Consistent builder usage in resource constraint test.The builder pattern change is applied correctly in this test case that verifies capacity exception handling.
66-67: LGTM! Builder chaining with capacity configuration looks good.The builder pattern is effectively used with method chaining to configure both shards and capacity, while maintaining the test's node reuse verification logic.
88-89: LGTM! Builder usage maintains test clarity.The builder pattern is consistently applied with proper method chaining, preserving the test's verification of round-robin node assignment behavior.
server/src/test/java/com/flipkart/varadhi/utils/VaradhiTopicFactoryTest.java (4)
4-11: LGTM! Good visibility and import organization.The change to package-private visibility is appropriate for test classes, and the new imports are well-organized and necessary for the lifecycle management functionality.
Also applies to: 29-29
31-39: LGTM! Good use of constants and Mockito annotations.The introduction of static final constants improves maintainability, and the Mockito annotations are properly used for dependency injection.
46-55: LGTM! Well-structured test setup.The setup method follows test best practices with proper mock initialization and clear mock behavior setup.
59-88: Add test cases for soft deletion scenarios.Given that this PR implements soft deletion support, consider adding test cases to verify:
- Topic creation with different lifecycle states
- Validation of soft-deleted topics
- Edge cases around topic restoration
Would you like me to help generate additional test cases for these scenarios?
server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java (5)
44-46: LGTM! Constants improve code maintainability.The replacement of mutable fields with static final constants enhances code clarity and thread safety.
81-84: LGTM! Improved error reporting in teardown.The addition of an assertion message helps with debugging when server closure fails.
106-110: LGTM! Better error handling for unsupported HTTP methods.The use of
UnsupportedOperationExceptionis more appropriate thanfail()for handling unsupported HTTP methods.
113-182: LGTM! Improved method naming and response handling.The changes enhance code clarity through:
- More descriptive method names (e.g.,
sendRequestWithBody→sendRequestWithEntity)- Robust null/empty response handling
- Better assertion messages for debugging
223-226: LGTM! Better response handling with PostResponseCapture.The introduction of
PostResponseCaptureclass improves code organization by encapsulating response data.entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (2)
20-20: LGTM! Well-designed status field addition.The new
statusfield follows the class's immutable design pattern and properly integrates with Lombok's @Getter annotation.
22-41: LGTM! Constructor changes are well-documented and maintain encapsulation.The private constructor properly initializes the new status field and includes comprehensive Javadoc.
pulsar/src/test/java/com/flipkart/varadhi/pulsar/services/PulsarTopicServiceTest.java (4)
53-53: LGTM! Consistent constant naming.The constant reference update follows Java naming conventions and maintains consistency across the codebase.
63-63: LGTM! Consistent constant naming.The constant reference update maintains consistency with the previous changes.
73-73: LGTM! Consistent constant naming.The constant reference update maintains consistency with the previous changes.
93-93: LGTM! Verify constant usage consistency.The constant reference update maintains consistency with the previous changes.
Let's verify that all occurrences of the old constant name have been updated:
✅ Verification successful
Constant Usage Consistency Verified
The updated constant is used consistently in the codebase. The search results confirm that instances referencing
Constants.DEFAULT_TOPIC_CAPACITYare correct, and the lower-casedefaultTopicCapacityentries remain only as configuration keys and variable names, which is expected.
PulsarTopicServiceTest.javaand several other test files correctly useConstants.DEFAULT_TOPIC_CAPACITY.- Other occurrences of
defaultTopicCapacityare in configuration files or local variable names, not as direct constant references.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of the old constant name # and verify the new constant is used consistently. echo "Checking for old constant name 'DefaultTopicCapacity'..." rg -i "DefaultTopicCapacity" echo "Verifying usage of new constant name 'DEFAULT_TOPIC_CAPACITY'..." rg "DEFAULT_TOPIC_CAPACITY"Length of output: 5840
server/src/test/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactoryTest.java (1)
1-23: LGTM!The file structure, package declaration, imports, and class inheritance are well-organized and appropriate for the test class.
entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java (3)
6-17: LGTM! Constants follow Java naming conventions.The constant renaming to uppercase and static imports organization look good.
34-55: LGTM! Comprehensive test coverage for core functionality.The tests effectively validate all essential properties including lifecycle status. Good use of
assertAllfor multiple assertions.
58-109: Consider adding test for invalid project/topic names.The tests cover the happy path and basic edge cases well. Consider adding tests for invalid project or topic names to ensure proper validation.
Let's check if there are any validation rules for project/topic names:
server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (3)
3-6: LGTM! Well-documented class structure.The class structure is well-organized with clear documentation and appropriate imports for the new lifecycle management features.
Also applies to: 18-34
38-51: Consider implementing partial failure handling.If
storageTopicService.create(...)succeeds butmetaStore.createTopic(...)fails, resources may become partially created. Consider implementing a compensating transaction strategy.Run this script to check for existing error handling patterns in the codebase:
#!/bin/bash # Description: Search for error handling patterns in storage topic creation # Test: Look for try-catch blocks or transaction handling around storage topic creation rg -A 10 "storageTopicService.*create.*try" .
122-151: LGTM! Robust restoration implementation.The restoration logic includes proper permission checks and state validation.
entities/src/main/java/com/flipkart/varadhi/entities/auth/ResourceAction.java (2)
16-21: Clarify the use ofResourceType.ROOTfor listing organizations
UsingResourceType.ROOTfor theORG_LISTaction (line 20) might be counterintuitive if the rest of the ORG actions useResourceType.ORG. Verify that the logic for listing organizations through the root resource is intentional and consistent with other resource types.
44-52: Validate the newRESTORE,CONSUME, andPRODUCEactions
These new actions (lines 49, 50, 51) introduce additional behavior for topic resources. Ensure proper access control checks and that the rest of the codebase consistently handles these actions (e.g., corresponding REST endpoints, service-layer logic).
[approve]server/src/main/java/com/flipkart/varadhi/web/entities/TopicResource.java (2)
33-55: Validate non-nullactionCodeat construction
The constructor (lines 33–55) introducesactionCodefor eachTopicResource. If the workflow requires an action code at creation time, consider adding an explicit null check to avoid accidental passing of null, which might lead to ambiguous states later.
88-114: Ensure consistent round-trip conversions
Thefrom(VaradhiTopic)method (lines 88–104) andtoVaradhiTopic()method (lines 108–114) correctly passactionCode. Double-check that upstream and downstream systems expect the updated code, including possible new states (e.g.,RESTORE). It may be beneficial to expand unit tests verifying thatactionCodeis preserved through these conversions.
[approve]server/src/main/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactory.java (2)
Line range hint
65-92: Confirm usage ofsubscriptionResource.getActionCode()
Thegetmethod (lines 65–92) now includes the subscription’sactionCode. Ensure that any subscription-creation flows calling this method provide a valid action code and that the rest of the system (e.g., ACL checks, user notifications) interprets it correctly.
Line range hint
96-125: Handle edge cases forgetSubscriptionShards
If the computed list of topic partitions (lines 116–125) is empty or null, the current logic proceeds without explicit handling. Consider adding a safeguard to prevent unexpected runtime errors when no partitions are available.entities/src/main/java/com/flipkart/varadhi/entities/ResourceActionRequest.java (1)
11-12: LGTM! Well-structured record with proper validation.The record is well-designed with appropriate validation annotations and implements the Validatable interface.
server/src/testE2E/java/com/flipkart/varadhi/OrgTests.java (1)
46-46: LGTM! Improved assertion readability.Good change to use
assertFalseinstead ofassertTrue(!x). This makes the test's intent clearer and follows testing best practices.entities/src/test/java/com/flipkart/varadhi/entities/ResourceActionRequestTest.java (1)
12-22: LGTM! Comprehensive test coverage.The test suite is well-structured with clear test cases covering essential scenarios.
Also applies to: 24-31, 33-40, 42-49, 51-58
entities/src/test/java/com/flipkart/varadhi/entities/LifecycleStatusTest.java (1)
12-21: LGTM! Well-structured test suite.The test suite provides good coverage of the LifecycleStatus functionality with clear and descriptive test cases.
Also applies to: 23-36, 38-48, 50-60, 62-67
entities/src/test/java/com/flipkart/varadhi/entities/auth/ResourceActionTest.java (1)
1-62: LGTM! Well-structured test class with comprehensive coverage.The test class effectively validates all aspects of ResourceAction using grouped assertions, which helps identify all potential failures in a single test run.
entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java (2)
97-101: Consider implementing finer-grained action codes.The commented-out action codes suggest a more granular approach to action tracking. This could provide better insights into who initiated actions and why. Consider if this granularity would be beneficial for auditing or debugging purposes.
Would you like to proceed with implementing the finer-grained action codes? This could help in:
- Distinguishing between direct user actions and admin-requested actions
- Tracking system-initiated actions separately from admin-forced actions
- Improving audit trails
8-115: LGTM! Well-structured lifecycle management implementation.The class provides a robust framework for managing entity lifecycles with:
- Clear state transitions with associated messages
- Flexible action code system
- Proper null handling in update methods
- Comprehensive documentation
server/src/main/java/com/flipkart/varadhi/web/entities/SubscriptionResource.java (1)
Line range hint
1-191: LGTM! Clean integration of lifecycle status.The changes properly integrate the new ActionCode while maintaining clean code principles:
- Comprehensive documentation
- Consistent factory methods
- Proper validation
- Clear separation of concerns
server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java (2)
208-208: LGTM! Improved error message clarity.The new error message "Grouped subscription cannot be created or updated for a non-grouped topic 'default.topic1'" is more descriptive and user-friendly compared to the previous version.
38-45: LGTM! Proper integration of lifecycle status and deletion types.The changes correctly integrate:
- LifecycleStatus.ActionCode for resource creation
- Explicit deletion type specification
Also applies to: 91-91, 134-134
entities/src/testFixtures/java/com/flipkart/varadhi/entities/SubscriptionUtils.java (1)
15-18: LGTM! Well-structured default constants.The default constants are well-named and appropriately scoped, providing clear configuration values for test scenarios.
server/src/testE2E/java/com/flipkart/varadhi/AuthZProviderTests.java (1)
56-59: LGTM! Clean integration of lifecycle status in test data.The addition of
LifecycleStatus.ActionCode.SYSTEM_ACTIONto test data setup is appropriate and maintains test coverage for the new feature.server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (1)
211-237: Enhance validation for restoration prerequisites.The restoration logic should validate the topic's existence and state before proceeding with subscription restoration.
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (3)
4-11: LGTM! Well-structured imports and clear class documentation.The new imports support the lifecycle management features, and the class documentation clearly describes its responsibilities.
Also applies to: 53-56
70-78: LGTM! Constructor and route definitions are well-structured.The constructor initialization is cleaner, and the new restore route is properly integrated with appropriate authorization.
Also applies to: 133-137
365-369: Consider adding tests for edge cases in lifecycle operations.While the implementation of helper methods is solid, consider adding tests for:
- Concurrent lifecycle operations
- Race conditions in state transitions
- Edge cases in validation logic
The past review comments already identified this gap. Adding these tests will improve the robustness of the implementation.
Also applies to: 460-466
server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java (1)
136-143: Add test for hard deleting a soft-deleted topic.The deletion tests don't cover the scenario of attempting to hard delete a topic that's already soft-deleted. This is an important state transition to verify.
The past review comments already identified this gap. The suggested test implementation in the past review is appropriate and matches the service's behavior.
server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (1)
119-130: LGTM! Comprehensive test coverage with clear test cases.The test implementation:
- Uses descriptive test method names
- Covers both success and failure scenarios
- Includes proper assertions and verifications
Also applies to: 298-313, 482-495
server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (1)
896-900: Enhance subscription comparison in assertSubscriptionsEqual.The helper method doesn't compare all relevant fields of the subscription. Consider adding comparisons for:
- Endpoint configuration
- Retry policy
- Consumption policy
- Version
- Status
The past review comments already identified this gap. The suggested implementation in the past review would improve the test's reliability.
server/src/test/java/com/flipkart/varadhi/web/entities/SubscriptionResourceTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/flipkart/varadhi/web/entities/SubscriptionResourceTest.java
Outdated
Show resolved
Hide resolved
pulsar/src/test/java/com/flipkart/varadhi/pulsar/PulsarStackProviderTest.java
Show resolved
Hide resolved
| @Test | ||
| void getShardCapacity_ValidInput_ReturnsCorrectCapacity() throws Exception { | ||
| Method method = | ||
| VaradhiSubscriptionFactory.class.getDeclaredMethod( | ||
| "getShardCapacity", TopicCapacityPolicy.class, | ||
| int.class | ||
| ); | ||
| method.setAccessible(true); | ||
|
|
||
| TopicCapacityPolicy result = | ||
| (TopicCapacityPolicy) method.invoke(varadhiSubscriptionFactory, CAPACITY_POLICY, 2); | ||
|
|
||
| assertNotNull(result); | ||
| assertEquals(CAPACITY_POLICY.getReadFanOut(), result.getReadFanOut()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider testing through public interfaces instead of using reflection.
Testing private methods through reflection might indicate that these methods should be extracted into a separate utility class or that we should test the behavior through public methods instead.
server/src/test/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactoryTest.java
Show resolved
Hide resolved
server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java
Show resolved
Hide resolved
server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (1)
486-499: 🛠️ Refactor suggestionEnhance restore operation test coverage.
The restore operation test only covers the success case. Based on the PR objectives and past review comments, additional test cases are needed.
Add test cases for:
- Restoring an already active subscription
- Restoring a non-existent subscription
- Concurrent restore operations
Example:
@Test void restoreSubscription_ConcurrentOperation_ThrowsException() throws InterruptedException { HttpRequest<Buffer> request = createRequest( HttpMethod.PATCH, buildSubscriptionUrl("sub1", PROJECT) + "/restore" ); doReturn(CompletableFuture.failedFuture( new ConcurrentModificationException("Concurrent modification detected") )).when(subscriptionService).restoreSubscription(any(), any(), any()); ErrorResponse response = sendRequestWithoutPayload( request, 409, "Concurrent modification detected", ErrorResponse.class ); assertEquals("Concurrent modification detected", response.reason()); }
🧹 Nitpick comments (14)
server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (1)
105-115: Consider adding request validation tests.The
createRoutehelper method handles body validation, but there are no tests verifying the behavior when invalid request bodies are received.Consider adding tests for:
@Test void createSubscription_InvalidRequestBody_ThrowsBadRequest() throws InterruptedException { HttpRequest<Buffer> request = createRequest(HttpMethod.POST, buildSubscriptionsUrl(PROJECT)); String invalidJson = "{invalid_json}"; ErrorResponse response = sendRequestWithInvalidBody( request, invalidJson, 400, "Invalid request body", ErrorResponse.class ); assertEquals("Invalid request body", response.reason()); }entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java (2)
38-134: Add test for subscription name validation.While the tests cover most validation scenarios, there's no test for invalid subscription names. Consider adding a test similar to the project and topic validation tests:
@Test void createSubscription_InvalidName_ThrowsException() { IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, () -> { VaradhiSubscription.of( "", "project1", "topic1", "description", true, DEFAULT_ENDPOINT, DEFAULT_RETRY_POLICY, DEFAULT_CONSUMPTION_POLICY, DEFAULT_SHARDS, Map.of("key", "value"), LifecycleStatus.ActionCode.SYSTEM_ACTION ); } ); assertEquals("Subscription name cannot be null or empty", exception.getMessage()); }
272-281: Improve integer overflow test precision.The current test uses an arbitrarily large number. Consider using
Integer.MAX_VALUE + "1"for a more precise test of integer overflow:- DEFAULT_SHARDS, Map.of("key", "999999999999999999999999"), LifecycleStatus.ActionCode.SYSTEM_ACTION + DEFAULT_SHARDS, Map.of("key", String.valueOf(Integer.MAX_VALUE) + "1"), LifecycleStatus.ActionCode.SYSTEM_ACTIONserver/src/test/java/com/flipkart/varadhi/web/WebTestBase.java (3)
48-56: Consider enhancing error handling and validation.While the error handling is improved, consider:
- Using a more specific exception type
- Adding parameter validation
public static <R, T> R jsonDeserialize(String data, Class<? extends Collection> collectionClass, Class<T> clazz) { + if (data == null || collectionClass == null || clazz == null) { + throw new IllegalArgumentException("Input parameters cannot be null"); + } try { JavaType valueType = JsonMapper.getMapper().getTypeFactory().constructCollectionType(collectionClass, clazz); return JsonMapper.getMapper().readValue(data, valueType); } catch (Exception e) { - throw new RuntimeException("Failed to deserialize JSON", e); + throw new JsonDeserializationException("Failed to deserialize JSON", e); } }
81-84: Consider using timeout constant for server close.For consistency with
LATCH_TIMEOUT, consider extracting the server close timeout to a constant.+ private static final long SERVER_CLOSE_TIMEOUT = 30L; + server.close().onComplete(asyncResult -> { assertTrue(asyncResult.succeeded(), "Server close failed"); latch.countDown(); }); - awaitLatch(latch); + assertTrue(latch.await(SERVER_CLOSE_TIMEOUT, TimeUnit.SECONDS), "Server close timeout");
223-226: Add documentation for PostResponseCapture class.Consider adding Javadoc to explain the purpose and thread-safety aspects of this class.
+ /** + * Thread-safe container for capturing HTTP response or error from async operations. + * Uses volatile fields to ensure visibility across threads. + * + * @param <T> The type of response being captured + */ private static class PostResponseCapture<T> { private volatile T response; private volatile Throwable throwable; }entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java (2)
99-104: Remove commented-out code.The commented-out action codes should be removed as they are not being used. If they are planned for future use, track them in a separate issue or TODO.
-// USER_INITIATED_ACTION, // Action initiated directly by the user. -// USER_REQUESTED_ADMIN_ACTION, // Action requested by the user to be performed by an admin. -// ADMIN_FORCED_ACTION, // Action intentionally performed by an admin. -// SYSTEM_ACTION; // Action performed by the system due to policy.
49-56: Consider adding state transition validation.The
updatemethod only validates against updating to the same state. Consider adding validation for allowed state transitions to prevent invalid state changes (e.g., from DELETED to CREATING).public void update(State state, String message, ActionCode actionCode) { if (this.state == state) { throw new IllegalArgumentException("Resource is already in " + state + " state"); } + if (!isValidStateTransition(this.state, state)) { + throw new IllegalArgumentException("Invalid state transition from " + this.state + " to " + state); + } this.state = state; this.message = message != null ? message : state.getDefaultMessage(); this.actionCode = actionCode; } +private boolean isValidStateTransition(State from, State to) { + // Define valid state transitions + switch (from) { + case CREATING: + return to == State.CREATED || to == State.CREATE_FAILED; + case UPDATING: + return to == State.UPDATED || to == State.UPDATE_FAILED; + case DELETING: + return to == State.DELETED || to == State.DELETE_FAILED; + // Add more cases as needed + default: + return true; + } +}server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java (2)
229-307: Consider refactoring to avoid testing private methods with reflection.The use of reflection to test
validateTopicForDeletionsuggests this method might be better placed in a separate validator class. This would improve testability and follow the Single Responsibility Principle.Consider extracting the validation logic:
public class TopicDeletionValidator { public void validateTopicForDeletion(String topicName, ResourceDeletionType deletionType) { // Move validation logic here } } public class VaradhiTopicService { private final TopicDeletionValidator validator; public void delete(String topicName, ResourceDeletionType deletionType, ResourceActionRequest actionRequest) { validator.validateTopicForDeletion(topicName, deletionType); // Rest of the delete logic } }
472-486: Simplify test setup with a builder pattern.The test setup methods could be simplified using a builder pattern, making the test setup more readable and maintainable.
Consider introducing a test builder:
public class VaradhiTopicTestBuilder { private String topicName = TOPIC_NAME; private String projectName = PROJECT_NAME; private TopicCapacityPolicy capacityPolicy = DEFAULT_CAPACITY_POLICY; private boolean isGrouped = false; private LifecycleStatus.ActionCode actionCode = LifecycleStatus.ActionCode.SYSTEM_ACTION; public static VaradhiTopicTestBuilder aVaradhiTopic() { return new VaradhiTopicTestBuilder(); } public VaradhiTopicTestBuilder withName(String name) { this.topicName = name; return this; } // Add more builder methods public VaradhiTopic build() { TopicResource resource = TopicResource.grouped( topicName, projectName, capacityPolicy, actionCode ); return varadhiTopicFactory.get(project, resource); } }server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (4)
169-173: Consider enhancing tearDown cleanup.While the current tearDown method closes the ZooKeeper client and server, consider adding cleanup for other resources:
- Clear any mocks
- Reset the meter registry
- Clean up any remaining test data in the meta store
582-784: Consider adding state transition assertions in deletion tests.While the tests cover the final states well, consider adding assertions to verify the intermediate state transitions during deletion operations. This would ensure the lifecycle status changes are happening in the correct order.
For example, in
deleteSubscription_HardDelete_Success, add assertions like:Future.fromCompletionStage( subscriptionService.deleteSubscription( subscriptionName, project1, REQUESTED_BY, ResourceDeletionType.HARD_DELETE, actionRequest )) .onComplete(ctx.succeeding(result -> { + // Verify state transition + verify(varadhiMetaStore, times(1)).updateSubscription(argThat(subscription -> + subscription.getStatus().getState() == LifecycleStatus.State.DELETING + )); ResourceNotFoundException exception = assertThrows( ResourceNotFoundException.class, () -> subscriptionService.getSubscription(subscriptionName) ); assertEquals("Subscription(%s) not found.".formatted(subscriptionName), exception.getMessage()); checkpoint.flag(); }));
955-967: Consider adding version comparison in assertSubscriptionsEqual.The assertSubscriptionsEqual method is comprehensive but doesn't compare the version field, which is important for tracking updates.
Add version comparison:
private void assertSubscriptionsEqual(VaradhiSubscription expected, VaradhiSubscription actual) { assertEquals(expected.getName(), actual.getName()); assertEquals(expected.getProject(), actual.getProject()); assertEquals(expected.getTopic(), actual.getTopic()); assertEquals(expected.getDescription(), actual.getDescription()); assertEquals(expected.isGrouped(), actual.isGrouped()); assertEquals(expected.getEndpoint().getProtocol(), actual.getEndpoint().getProtocol()); assertEquals(expected.getRetryPolicy(), actual.getRetryPolicy()); assertEquals(expected.getConsumptionPolicy(), actual.getConsumptionPolicy()); assertEquals(expected.getShards().getShardCount(), actual.getShards().getShardCount()); assertEquals(expected.getStatus().getState(), actual.getStatus().getState()); assertEquals(expected.getProperties(), actual.getProperties()); + assertEquals(expected.getVersion(), actual.getVersion()); }
854-904: Consider validating restoration request details.While the restoration tests cover the core functionality, consider adding assertions to validate the details of the restoration request.
Add request validation in
restoreSubscription_Success:assertDoesNotThrow(() -> { VaradhiSubscription restoredSubscription = result.get(); assertEquals(LifecycleStatus.State.CREATED, restoredSubscription.getStatus().getState()); + // Validate restoration request details + assertEquals(LifecycleStatus.ActionCode.SYSTEM_ACTION, actionRequest.actionCode()); + assertEquals("Restore", actionRequest.reason()); + // Validate status history + assertTrue(restoredSubscription.getStatus().getHistory().stream() + .anyMatch(h -> h.getState() == LifecycleStatus.State.INACTIVE)); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/LifecycleStatusTest.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java(5 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (11)
server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (2)
70-103: LGTM! Well-organized route configuration.The
configureRoutesmethod provides a clear and maintainable way to set up all subscription-related routes.
301-369: Comprehensive deletion test coverage.The deletion tests thoroughly cover:
- Soft delete
- Hard delete
- Default deletion type
- Invalid deletion type handling
This aligns well with the PR objective of implementing both soft and hard deletion functionalities.
entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java (2)
16-37: LGTM! Well-structured test constants.The default test values are well-defined and provide a good foundation for testing various scenarios.
211-225: Add tests for invalid restore state transitions.The current restore test only covers the successful case. Given that this PR implements soft-deletion support, it's crucial to test invalid restore scenarios.
As suggested in the previous review, add tests to verify that:
- Restoration is only possible from INACTIVE state
- Restoration attempts from other states (e.g., CREATING, DELETING) throw appropriate exceptions
server/src/test/java/com/flipkart/varadhi/web/WebTestBase.java (4)
44-46: LGTM! Good use of constants.The replacement of mutable fields with static final constants improves code maintainability and prevents accidental modifications.
106-110: LGTM! Good improvements to HTTP method handling.The addition of PATCH support and explicit error handling for unsupported methods improves the API's robustness.
113-183: LGTM! Excellent improvements to method naming and null handling.The changes improve code clarity through:
- More descriptive method names (e.g.,
sendRequestWithPayloadinstead ofsendRequestWithByteBufferBody)- Better null handling using Optional
- More informative assertion messages
208-221: LGTM! Good improvements to error handling.The use of constant timeout and improved error messages in assertions enhances debugging capabilities.
entities/src/test/java/com/flipkart/varadhi/entities/LifecycleStatusTest.java (1)
1-82: Well-structured test suite with comprehensive coverage!The test suite follows best practices with:
- Clear and descriptive test method names
- Single responsibility per test
- Good coverage of edge cases and error conditions
- Proper use of assertAll for multiple assertions
entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java (1)
111-148: Test coverage for hard deleting a soft-deleted topic is missing.As mentioned in the past review comments, we need to verify that a soft-deleted topic can be hard deleted.
Add a test case to verify this scenario:
@Test void deleteVaradhiTopic_HardDeleteSoftDeletedTopic_SuccessfulDelete() { VaradhiTopic varadhiTopic = mockDeleteSetup(); varadhiTopic.markInactive(LifecycleStatus.ActionCode.SYSTEM_ACTION, "Soft deleted"); varadhiTopicService.delete(varadhiTopic.getName(), ResourceDeletionType.HARD_DELETE, null); verify(storageTopicService, times(1)).delete(pulsarStorageTopic.getName(), project); verify(metaStore, times(1)).deleteTopic(varadhiTopic.getName()); }server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (1)
1-89: LGTM! Well-organized imports and class setup.The imports are properly organized, and the test class is correctly set up with the necessary dependencies and annotations.
| @Test | ||
| void startSubscription_ValidRequest_TriggersStart() throws InterruptedException { | ||
| HttpRequest<Buffer> request = createRequest( | ||
| HttpMethod.POST, | ||
| buildSubscriptionUrl("sub1", PROJECT) + "/start" | ||
| ); | ||
|
|
||
| doReturn(CompletableFuture.completedFuture(null)).when(subscriptionService).start(any(), any()); | ||
|
|
||
| sendRequestWithoutPayload(request, null); | ||
|
|
||
| verify(subscriptionService, times(1)).start(any(), any()); | ||
| } | ||
|
|
||
| @Test | ||
| void stopSubscription_ValidRequest_TriggersStop() throws InterruptedException { | ||
| HttpRequest<Buffer> request = createRequest( | ||
| HttpMethod.POST, | ||
| buildSubscriptionUrl("sub1", PROJECT) + "/stop" | ||
| ); | ||
|
|
||
| doReturn(CompletableFuture.completedFuture(null)).when(subscriptionService).stop(any(), any()); | ||
|
|
||
| sendRequestWithoutPayload(request, null); | ||
|
|
||
| verify(subscriptionService, times(1)).stop(any(), any()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance lifecycle operation test coverage.
The start/stop operation tests only cover success cases. Additional test cases are needed for error scenarios.
Add test cases for:
- Starting/stopping a non-existent subscription
- Starting an already started subscription
- Stopping an already stopped subscription
- Error handling during start/stop operations
Example:
@Test
void startSubscription_AlreadyStarted_ThrowsException() throws InterruptedException {
HttpRequest<Buffer> request = createRequest(
HttpMethod.POST,
buildSubscriptionUrl("sub1", PROJECT) + "/start"
);
doReturn(CompletableFuture.failedFuture(
new IllegalStateException("Subscription is already started")
)).when(subscriptionService).start(any(), any());
ErrorResponse response = sendRequestWithoutPayload(
request, 400, "Subscription is already started",
ErrorResponse.class
);
assertEquals("Subscription is already started", response.reason());
}
entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java
Show resolved
Hide resolved
entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java
Outdated
Show resolved
Hide resolved
entities/src/main/java/com/flipkart/varadhi/entities/ResourceActionRequest.java
Outdated
Show resolved
Hide resolved
entities/src/test/java/com/flipkart/varadhi/entities/auth/ResourceActionTest.java
Show resolved
Hide resolved
entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java
Outdated
Show resolved
Hide resolved
entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java
Outdated
Show resolved
Hide resolved
- Added new end-to-end (E2E) tests to improve test coverage. - Refactored E2EBase to enhance code maintainability and readability. - Resolved PR review comments by making necessary code adjustments and improvements.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
server/src/test/java/com/flipkart/varadhi/web/entities/SubscriptionResourceTest.java (1)
15-74: 🛠️ Refactor suggestionAdd test coverage for soft-deletion functionality.
The test class is missing coverage for the soft-deletion functionality mentioned in the PR objectives. Consider adding test cases for:
- Converting a soft-deleted
VaradhiSubscriptiontoSubscriptionResource- Different lifecycle status states and action codes
- Restoration scenarios
- Invalid state transitions
This will ensure comprehensive test coverage for the new lifecycle management features.
server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (1)
249-257: 🛠️ Refactor suggestionAdd negative test cases for restore operation.
The restore operation only tests the happy path. Consider adding test cases for error scenarios.
Add these test methods:
@Test void restoreTopic_WithNonExistentTopic_ShouldReturnNotFound() throws InterruptedException { HttpRequest<Buffer> request = createRequest(HttpMethod.PATCH, getTopicUrl(project) + "/restore"); doThrow(new TopicNotFoundException("Topic not found")) .when(varadhiTopicService) .restore(any(), any()); HttpResponse<Buffer> response = sendRequestWithoutPayload(request, Buffer.class); assertEquals(404, response.statusCode()); assertErrorResponse(response, "Topic not found"); } @Test void restoreTopic_WithActiveTopicShouldReturnBadRequest() throws InterruptedException { HttpRequest<Buffer> request = createRequest(HttpMethod.PATCH, getTopicUrl(project) + "/restore"); doThrow(new IllegalStateException("Topic is already active")) .when(varadhiTopicService) .restore(any(), any()); HttpResponse<Buffer> response = sendRequestWithoutPayload(request, Buffer.class); assertEquals(400, response.statusCode()); assertErrorResponse(response, "Topic is already active"); }entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java (1)
75-88: 🛠️ Refactor suggestionConsider refining the state enum based on previous discussions.
Based on the previous discussions, some states like "UPDATED" and "DELETED" might be redundant since:
- "CREATED" and "ACTIVE" are functionally equivalent
- "DELETED" is not a valid state
- "UPDATED" is functionally equivalent to "CREATED"
Consider simplifying to essential states only.
server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (1)
338-355:⚠️ Potential issueEnhance error handling in handleHardDelete.
The hard deletion process could leave the subscription in an inconsistent state if
deProvisionfails after updating the status but before deleting from the store.private void handleHardDelete( VaradhiSubscription subscription, Project subProject, ResourceActionRequest actionRequest ) { subscription.markDeleting(actionRequest.actorCode(), actionRequest.message()); - metaStore.updateSubscription(subscription); + VaradhiSubscription originalState = subscription.copy(); try { + metaStore.updateSubscription(subscription); shardProvisioner.deProvision(subscription, subProject); metaStore.deleteSubscription(subscription.getName()); log.info("Subscription '{}' deleted successfully.", subscription.getName()); } catch (Exception e) { log.error("Failed to hard delete subscription '{}'.", subscription.getName(), e); - subscription.markDeleteFailed(e.getMessage()); - metaStore.updateSubscription(subscription); + try { + subscription.markDeleteFailed(e.getMessage()); + metaStore.updateSubscription(subscription); + } catch (Exception updateError) { + log.error("Failed to update subscription state after deletion failure", updateError); + // Attempt to restore original state + try { + metaStore.updateSubscription(originalState); + } catch (Exception restoreError) { + log.error("Failed to restore subscription to original state", restoreError); + } + } throw e; } }server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (1)
500-526: 🛠️ Refactor suggestionEnhance lifecycle operation test coverage.
The start/stop operation tests only cover the success path. Additional test cases are needed for error scenarios.
Add test cases for:
- Starting/stopping a non-existent subscription
- Starting an already started subscription
- Stopping an already stopped subscription
- Error handling during start/stop operations
Example:
@Test void startSubscription_AlreadyStarted_ThrowsException() throws InterruptedException { HttpRequest<Buffer> request = createRequest( HttpMethod.POST, buildSubscriptionUrl("sub1", PROJECT) + "/start" ); doReturn(CompletableFuture.failedFuture( new IllegalStateException("Subscription is already started") )).when(subscriptionService).start(any(), any()); ErrorResponse response = sendRequestWithoutPayload( request, 400, "Subscription is already started", ErrorResponse.class ); assertEquals("Subscription is already started", response.reason()); }server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (1)
955-967: 🛠️ Refactor suggestionEnhance subscription comparison in assertSubscriptionsEqual.
The helper method doesn't compare all relevant fields of the subscription.
Consider adding comparisons for:
- Version
- Internal composite subscription details
- Retry subscription details
private void assertSubscriptionsEqual(VaradhiSubscription expected, VaradhiSubscription actual) { assertEquals(expected.getName(), actual.getName()); assertEquals(expected.getProject(), actual.getProject()); assertEquals(expected.getTopic(), actual.getTopic()); assertEquals(expected.getDescription(), actual.getDescription()); assertEquals(expected.isGrouped(), actual.isGrouped()); assertEquals(expected.getEndpoint().getProtocol(), actual.getEndpoint().getProtocol()); assertEquals(expected.getRetryPolicy(), actual.getRetryPolicy()); assertEquals(expected.getConsumptionPolicy(), actual.getConsumptionPolicy()); assertEquals(expected.getShards().getShardCount(), actual.getShards().getShardCount()); assertEquals(expected.getStatus().getState(), actual.getStatus().getState()); assertEquals(expected.getProperties(), actual.getProperties()); + assertEquals(expected.getVersion(), actual.getVersion()); + assertEquals(expected.getShards().getShard(0).getDeadLetterSubscription(), + actual.getShards().getShard(0).getDeadLetterSubscription()); + assertEquals(expected.getShards().getShard(0).getRetrySubscription(), + actual.getShards().getShard(0).getRetrySubscription()); }
🧹 Nitpick comments (28)
server/build.gradle (1)
104-108: TestE2E Task Configuration Enhancement:
The newly addedtestE2Etask is correctly configured withuseJUnitPlatform()to leverage JUnit 5 for end-to-end testing. The inclusion of the JVM argumentjvmArgs '--add-opens', 'java.base/java.net=ALL-UNNAMED'ensures that reflective access to
java.netis permitted for unnamed modules, which is likely required by some test dependencies.Recommendation:
It would improve maintainability to add an inline comment explaining the need for thisjvmArgsconfiguration (e.g., linking it to addressing module access issues), so that future maintainers understand the rationale.entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (3)
10-12: Enhance class documentation.While the documentation is clear, consider adding more details about:
- The purpose and significance of the lifecycle status
- Valid state transitions
- The relationship between topics and their lifecycle states
Also applies to: 20-20
43-62: Add initial state information to documentation.The documentation should mention that the topic is created in an ACTIVE state.
Add this to the method documentation:
* @param actorCode the actor code indicating the reason for the state + * @return a new VaradhiTopic instance in ACTIVE state - * @return a new VaradhiTopic instance */
107-135: Consider adding state transition validation.The lifecycle management methods should validate state transitions to prevent invalid state changes. For example:
- Prevent marking an already active topic as active
- Prevent marking an already inactive topic as inactive
Consider adding validation like this:
public void markActive(LifecycleStatus.ActorCode actorCode, String message) { + if (isActive()) { + throw new IllegalStateException("Topic is already active"); + } this.status.update(LifecycleStatus.State.ACTIVE, message, actorCode); } public void markInactive(LifecycleStatus.ActorCode actorCode, String message) { + if (!isActive()) { + throw new IllegalStateException("Topic is already inactive"); + } this.status.update(LifecycleStatus.State.INACTIVE, message, actorCode); }server/src/test/java/com/flipkart/varadhi/utils/VaradhiTopicFactoryTest.java (2)
31-40: Consider more descriptive names for test constants.While converting instance variables to static constants is good, consider making the names more descriptive:
REGION→TEST_REGIONTOPIC_NAME→TEST_TOPIC_NAME- private static final String REGION = "local"; - private static final String TOPIC_NAME = "testTopic"; + private static final String TEST_REGION = "local"; + private static final String TEST_TOPIC_NAME = "testTopic";
90-112: Consider alternatives to testing private methods with reflection.While the test is well-written, using reflection to test private methods is generally not recommended as it:
- Breaks encapsulation
- Makes tests brittle to refactoring
- May indicate that the tested behavior should be in a separate class
Consider either:
- Testing the behavior through public methods
- Extracting the logic to a new class if it's complex enough to warrant separate testing
server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (2)
88-115: Consider extracting route setup into separate methods.The
setupRoutesmethod is handling multiple responsibilities. Consider breaking it down into smaller, focused methods for better readability and maintainability.- private void setupRoutes() { + private void setupRoutes() { + setupTopicCreationRoute(); + setupTopicRetrievalRoutes(); + setupTopicDeletionRoute(); + setupTopicRestorationRoute(); + setupFailureHandlers(); + } + + private void setupTopicCreationRoute() { + router.post("/projects/:project/topics") + .handler(bodyHandler) + .handler(ctx -> { + topicHandlers.setRequestBody(ctx); + ctx.next(); + }) + .handler(ctx -> { + requestTelemetryConfigurator.addRequestSpanAndLog(ctx, "CreateTopic", TelemetryType.ALL); + ctx.next(); + }) + .handler(wrapBlocking(topicHandlers::create)); + }
259-272: Consider adding validation for topic name in verifyDeleteRequest.The
verifyDeleteRequesthelper method should verify that the correct topic name is being passed to the delete operation.private void verifyDeleteRequest(String deletionType, ResourceDeletionType expectedDeletionType) throws InterruptedException { String url = getTopicUrl(project); if (deletionType != null) { url += "?deletionType=" + deletionType; } HttpRequest<Buffer> request = createRequest(HttpMethod.DELETE, url); doNothing().when(varadhiTopicService).delete(any(), eq(expectedDeletionType), any()); sendRequestWithoutPayload(request, null); - verify(varadhiTopicService).delete(any(), eq(expectedDeletionType), any()); + String expectedTopicName = String.join(".", project.getName(), TOPIC_NAME); + verify(varadhiTopicService).delete(eq(expectedTopicName), eq(expectedDeletionType), any()); }server/src/main/java/com/flipkart/varadhi/web/entities/TopicResource.java (2)
14-16: Enhance class documentation.The class-level documentation could be more descriptive. Consider adding details about:
- The purpose and lifecycle states of a topic resource
- The relationship with
VaradhiTopic- The significance of grouped vs ungrouped topics
/** * Represents a topic resource in the Varadhi system. + * + * A TopicResource is a web entity that represents the configuration and state of a topic, + * including its lifecycle status. It can be either grouped or ungrouped, affecting how + * messages are processed. This class provides conversion methods to and from VaradhiTopic + * and maintains the lifecycle state through actorCode. */
88-115: Enhance documentation of lifecycle state conversion.While the implementation is correct, the documentation could better explain how lifecycle states are handled during conversion.
/** * Creates a TopicResource instance from a VaradhiTopic instance. * * @param varadhiTopic the VaradhiTopic instance + * @throws IllegalStateException if the topic's lifecycle status is invalid * * @return a new TopicResource instance */ /** * Converts this TopicResource instance to a VaradhiTopic instance. * + * @throws IllegalStateException if the topic's lifecycle status is invalid * @return a new VaradhiTopic instance */entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java (3)
19-31: LGTM! Well-structured test helpers.The helper class and method improve test maintainability. Consider adding Javadoc to document the purpose of these helpers.
Add documentation:
+/** + * Helper class for testing StorageTopic functionality. + */ @EqualsAndHashCode(callSuper = true) public static class DummyStorageTopic extends StorageTopic { +/** + * Creates a VaradhiTopic instance with default test parameters. + * @param grouped whether the topic should be grouped + * @return a new VaradhiTopic instance + */ private VaradhiTopic createDefaultVaradhiTopic(boolean grouped) {
58-109: LGTM! Good coverage of edge cases.The tests thoroughly validate topic name building, internal topic management, and region handling, including edge cases.
Consider adding a test for invalid region names (e.g., null, empty string) to improve edge case coverage:
+@Test +void getProduceTopicForRegion_WithInvalidRegion_ReturnsNull() { + VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false); + + assertAll( + () -> assertNull(varadhiTopic.getProduceTopicForRegion(null), "Null region should return null"), + () -> assertNull(varadhiTopic.getProduceTopicForRegion(""), "Empty region should return null") + ); +}
111-148: LGTM! Comprehensive lifecycle status testing.The tests thoroughly validate lifecycle status transitions and actor codes, aligning well with the soft-deletion support objectives.
Consider adding these test cases to improve coverage:
- Attempting to mark an active topic as active (idempotency)
- Attempting to mark an inactive topic as inactive (idempotency)
- Validating the reason message in the lifecycle status
Example:
+@Test +void markActive_WhenAlreadyActive_MaintainsActiveStatus() { + VaradhiTopic varadhiTopic = createDefaultVaradhiTopic(false); + String reason = "Already active"; + + varadhiTopic.markActive(LifecycleStatus.ActorCode.SYSTEM_ACTION, reason); + + assertAll( + () -> assertTrue(varadhiTopic.isActive(), "Should remain active"), + () -> assertEquals(reason, varadhiTopic.getStatus().getReason(), "Reason should be updated") + ); +}server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java (4)
78-89: Consider improving the readability of the setUp method.The setup method could be more readable by extracting the topic name construction logic into a helper method.
@BeforeEach public void setUp() { MockitoAnnotations.openMocks(this); varadhiTopicFactory = spy(new VaradhiTopicFactory(storageTopicFactory, REGION, DEFAULT_CAPACITY_POLICY)); project = Project.of("default", "", "public", "public"); - vTopicName = String.format("%s.%s", project.getName(), TOPIC_NAME); - String pTopicName = String.format("persistent://%s/%s/%s", project.getOrg(), project.getName(), vTopicName); + vTopicName = constructVaradhiTopicName(project.getName(), TOPIC_NAME); + String pTopicName = constructPulsarTopicName(project, vTopicName); pulsarStorageTopic = PulsarStorageTopic.of(pTopicName, 1, DEFAULT_CAPACITY_POLICY); doReturn(pulsarStorageTopic).when(storageTopicFactory) .getTopic(vTopicName, project, DEFAULT_CAPACITY_POLICY, InternalQueueCategory.MAIN); } +private String constructVaradhiTopicName(String projectName, String topicName) { + return String.format("%s.%s", projectName, topicName); +} + +private String constructPulsarTopicName(Project project, String varadhiTopicName) { + return String.format("persistent://%s/%s/%s", project.getOrg(), project.getName(), varadhiTopicName); +}
91-133: Add test coverage for additional edge cases in topic creation.Consider adding tests for the following scenarios:
- Creating a topic with an invalid name
- Creating a topic that already exists
@Test void createVaradhiTopic_TopicAlreadyExists_ThrowsException() { VaradhiTopic varadhiTopic = createVaradhiTopicMock(); doThrow(new VaradhiException("Topic already exists")) .when(metaStore).createTopic(varadhiTopic); Exception exception = assertThrows( VaradhiException.class, () -> varadhiTopicService.create(varadhiTopic, project) ); assertEquals("Topic already exists", exception.getMessage()); } @Test void createVaradhiTopic_InvalidTopicName_ThrowsException() { TopicResource topicResource = TopicResource.grouped( "invalid/topic/name", project.getName(), DEFAULT_CAPACITY_POLICY, LifecycleStatus.ActorCode.SYSTEM_ACTION ); VaradhiTopic varadhiTopic = varadhiTopicFactory.get(project, topicResource); assertThrows(IllegalArgumentException.class, () -> varadhiTopicService.create(varadhiTopic, project)); }
309-378: Add test coverage for restoring topics with active subscriptions.Consider adding a test to verify the behavior when restoring a topic that has active subscriptions.
@Test void restoreVaradhiTopic_WithActiveSubscriptions_SuccessfulRestore() { VaradhiTopic varadhiTopic = createVaradhiTopicMock(); varadhiTopic.markInactive(LifecycleStatus.ActorCode.SYSTEM_ACTION, "message"); when(metaStore.getTopic(varadhiTopic.getName())).thenReturn(varadhiTopic); when(metaStore.getAllSubscriptionNames()).thenReturn(List.of("subscription1")); when(metaStore.getSubscription("subscription1")).thenReturn(subscription); when(subscription.getTopic()).thenReturn(varadhiTopic.getName()); when(subscription.getStatus()).thenReturn(status); when(status.getState()).thenReturn(LifecycleStatus.State.CREATED); ResourceActionRequest actionRequest = new ResourceActionRequest( LifecycleStatus.ActorCode.SYSTEM_ACTION, "message" ); assertDoesNotThrow(() -> varadhiTopicService.restore(varadhiTopic.getName(), actionRequest)); verify(metaStore, times(1)).updateTopic(varadhiTopic); assertTrue(varadhiTopic.isActive()); }
472-499: Add Javadoc to helper methods.Consider adding Javadoc to document the purpose and parameters of helper methods.
+/** + * Creates a mock VaradhiTopic instance with default settings. + * + * @return A mock VaradhiTopic instance + */ private VaradhiTopic createVaradhiTopicMock() { // ... existing code ... } +/** + * Sets up common mocks for delete operations. + * + * @return A mock VaradhiTopic instance configured for deletion + */ private VaradhiTopic mockDeleteSetup() { // ... existing code ... } +/** + * Sets up mock topics with specified names and active states. + * + * @param projectName The name of the project + * @param topicNames List of topic names to mock + * @param topicStatuses List of boolean flags indicating if each topic is active + * @throws IllegalArgumentException if topicNames and topicStatuses have different sizes + */ private void setupMockTopics(String projectName, List<String> topicNames, List<Boolean> topicStatuses) { // ... existing code ... }server/src/main/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactory.java (2)
302-302: Address TODO comment about project ownership.The TODO raises an important architectural consideration about project ownership for Retry and DLT topics.
Consider implementing a project mapping strategy that:
- Maintains clear ownership boundaries
- Handles cross-project permissions
- Implements audit logging for cross-project operations
169-184: Consider adding parallel processing for multi-shard creation.The
getMultiShardmethod creates shards sequentially. For better performance with large shard counts, consider parallel processing.Here's a suggested parallel implementation:
private SubscriptionMultiShard getMultiShard( String subName, List<TopicPartitions<StorageTopic>> partitions, TopicCapacityPolicy capacity, Project subProject, ConsumptionPolicy consumptionPolicy, RetryPolicy retryPolicy ) { - Map<Integer, SubscriptionUnitShard> subShards = new HashMap<>(); - for (int shardId = 0; shardId < partitions.size(); shardId++) { - subShards.put( - shardId, getSingleShard( - subName, shardId, partitions.get(shardId), capacity, subProject, - consumptionPolicy, retryPolicy - ) - ); - } + Map<Integer, SubscriptionUnitShard> subShards = partitions.parallelStream() + .collect(HashMap::new, + (map, partition) -> { + int shardId = partitions.indexOf(partition); + map.put(shardId, getSingleShard( + subName, shardId, partition, capacity, subProject, + consumptionPolicy, retryPolicy)); + }, + Map::putAll); return new SubscriptionMultiShard(subShards); }entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java (1)
100-104: Remove commented-out actor codes.The commented-out actor codes appear to be from a previous iteration. Since they're no longer needed, they should be removed to maintain code cleanliness.
entities/src/main/java/com/flipkart/varadhi/entities/VaradhiSubscription.java (1)
246-251: Reconsider empty properties validation.The
validatePropertiesmethod throws an exception if properties are empty. Consider allowing empty properties as they might be valid in some cases.- if (properties == null || properties.isEmpty()) { + if (properties == null) { throw new IllegalArgumentException(PROPERTIES_ERROR); } - return properties; + return new HashMap<>(properties);entities/src/testFixtures/java/com/flipkart/varadhi/entities/SubscriptionUtils.java (2)
15-18: Consider making default constants public.These constants could be useful for other test classes. Consider making them public and documenting their intended use.
- private static final int DEFAULT_NUM_SHARDS = 2; - private static final int DEFAULT_QPS = 1000; - private static final int DEFAULT_THROUGHPUT_KBPS = 20000; - private static final int DEFAULT_READ_FANOUT = 2; + public static final int DEFAULT_NUM_SHARDS = 2; + public static final int DEFAULT_QPS = 1000; + public static final int DEFAULT_THROUGHPUT_KBPS = 20000; + public static final int DEFAULT_READ_FANOUT = 2;
279-303: Extract default value initialization to separate methods.The
buildmethod contains hardcoded default value initialization. Consider extracting these to separate methods for better maintainability.+ private TopicCapacityPolicy getDefaultCapacity() { + return getCapacity(DEFAULT_QPS, DEFAULT_THROUGHPUT_KBPS, DEFAULT_READ_FANOUT); + } + + private String getDefaultDescription(String name, String subscribedTopic) { + return "Test Subscription " + name + " Subscribed to " + subscribedTopic; + } + public VaradhiSubscription build(String name, String subProject, String subscribedTopic) { if (subCapacity == null) { - subCapacity = getCapacity(DEFAULT_QPS, DEFAULT_THROUGHPUT_KBPS, DEFAULT_READ_FANOUT); + subCapacity = getDefaultCapacity(); } // ... rest of the method return VaradhiSubscription.of( name, subProject, subscribedTopic, Optional.ofNullable(description) - .orElse("Test Subscription " + name + " Subscribed to " + subscribedTopic), + .orElse(getDefaultDescription(name, subscribedTopic)),server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java (2)
154-156: Add validation for empty message in create method.The
createmethod sets the actor code but doesn't validate the message parameter. Consider adding validation to ensure the message is not empty or contains only whitespace when provided.String requestedBy = ctx.getIdentityOrDefault(); LifecycleStatus.ActorCode actorCode = isVaradhiAdmin(requestedBy) ? LifecycleStatus.ActorCode.ADMIN_ACTION : LifecycleStatus.ActorCode.USER_ACTION; +String message = ctx.queryParam(QUERY_PARAM_MESSAGE).stream().findFirst().orElse(""); +if (!message.isBlank()) { + topicResource.setMessage(message); +} topicResource.setActorCode(actorCode);
182-190: Consider adding retry mechanism for delete operation.The delete operation could fail due to transient issues. Consider implementing a retry mechanism with exponential backoff for better reliability.
public void delete(RoutingContext ctx) { ResourceDeletionType deletionType = ctx.queryParam(QUERY_PARAM_DELETION_TYPE).stream() .map(ResourceDeletionType::fromValue) .findFirst() .orElse(ResourceDeletionType.SOFT_DELETE); ResourceActionRequest actionRequest = createResourceActionRequest(ctx); - - varadhiTopicService.delete(getVaradhiTopicName(ctx), deletionType, actionRequest); + retryWithBackoff(() -> + varadhiTopicService.delete(getVaradhiTopicName(ctx), deletionType, actionRequest), + 3, // maxRetries + 1000 // initialDelayMs + ); ctx.endApi(); } +private void retryWithBackoff(Runnable operation, int maxRetries, long initialDelayMs) { + int retryCount = 0; + while (true) { + try { + operation.run(); + return; + } catch (Exception e) { + if (++retryCount > maxRetries) { + throw e; + } + try { + Thread.sleep(initialDelayMs * (1L << (retryCount - 1))); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + throw new RuntimeException("Interrupted during retry", ie); + } + } + } +}server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java (1)
149-152: Add error handling for cleanup operations.The
cleanupTopicmethod should handle exceptions gracefully to prevent test failures due to cleanup issues.public static void cleanupTopic(String topicName, Project project) { + try { makeDeleteRequest( getTopicsUri(project, topicName), ResourceDeletionType.HARD_DELETE.toString(), EXPECTED_STATUS_OK); + } catch (Exception e) { + log.warn("Failed to cleanup topic {}: {}", topicName, e.getMessage()); + } }server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (1)
485-498: Add test for concurrent restore operations.The restore operation tests should verify that concurrent restore operations are handled correctly.
Add test case to verify that concurrent restore operations are properly synchronized:
@Test void restoreSubscription_ConcurrentRestore_HandledCorrectly() throws InterruptedException { HttpRequest<Buffer> request = createRequest( HttpMethod.PATCH, buildSubscriptionUrl("sub1", PROJECT) + "/restore" ); doReturn(CompletableFuture.failedFuture( new IllegalStateException("Concurrent restore operation in progress") )).when(subscriptionService).restoreSubscription(any(), any(), any()); ErrorResponse response = sendRequestWithoutPayload( request, 409, "Concurrent restore operation in progress", ErrorResponse.class ); assertEquals("Concurrent restore operation in progress", response.reason()); }server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java (1)
787-798: Add test for concurrent start operations.The start operation tests should verify that concurrent start operations are handled correctly.
Add test case to verify that concurrent start operations are properly synchronized:
@Test void startSubscription_ConcurrentStart_HandledCorrectly() { doReturn(unGroupedTopic).when(varadhiMetaStore).getTopic(unGroupedTopic.getName()); subscriptionService.createSubscription(unGroupedTopic, subscription1, project1); CompletableFuture<SubscriptionOperation> startFuture = CompletableFuture.failedFuture( new IllegalStateException("Concurrent start operation in progress") ); doReturn(startFuture).when(controllerRestApi) .startSubscription(subscription1.getName(), REQUESTED_BY); ExecutionException exception = assertThrows( ExecutionException.class, () -> subscriptionService.start(subscription1.getName(), REQUESTED_BY).get() ); assertEquals( "Concurrent start operation in progress", exception.getCause().getMessage() ); verify(controllerRestApi, times(1)) .startSubscription(subscription1.getName(), REQUESTED_BY); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
entities/src/main/java/com/flipkart/varadhi/entities/LifecycleStatus.java(1 hunks)entities/src/main/java/com/flipkart/varadhi/entities/VaradhiSubscription.java(4 hunks)entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/LifecycleStatusTest.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java(1 hunks)entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java(1 hunks)entities/src/testFixtures/java/com/flipkart/varadhi/entities/SubscriptionUtils.java(5 hunks)producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java(3 hunks)server/build.gradle(1 hunks)server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java(2 hunks)server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java(1 hunks)server/src/main/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactory.java(12 hunks)server/src/main/java/com/flipkart/varadhi/web/entities/ResourceActionRequest.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/entities/SubscriptionResource.java(5 hunks)server/src/main/java/com/flipkart/varadhi/web/entities/TopicResource.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java(3 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java(4 hunks)server/src/test/java/com/flipkart/varadhi/services/SubscriptionServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/utils/VaradhiTopicFactoryTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/entities/ResourceActionRequestTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/entities/SubscriptionResourceTest.java(1 hunks)server/src/test/java/com/flipkart/varadhi/web/entities/TopicResourceTest.java(1 hunks)server/src/testE2E/java/com/flipkart/varadhi/AuthZProviderTests.java(3 hunks)server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java(3 hunks)server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java(12 hunks)server/src/testE2E/java/com/flipkart/varadhi/TopicTests.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- server/src/testE2E/java/com/flipkart/varadhi/TopicTests.java
- server/src/testE2E/java/com/flipkart/varadhi/AuthZProviderTests.java
- entities/src/test/java/com/flipkart/varadhi/entities/VaradhiSubscriptionTest.java
- entities/src/test/java/com/flipkart/varadhi/entities/LifecycleStatusTest.java
- server/src/test/java/com/flipkart/varadhi/web/entities/TopicResourceTest.java
- producer/src/test/java/com/flipkart/varadhi/services/ProducerServiceTests.java
- server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (27)
server/src/test/java/com/flipkart/varadhi/web/entities/SubscriptionResourceTest.java (1)
1-14: Well-structured test class with clear organization.The test class follows good practices with proper imports, clear test method names, and effective use of JUnit 5 features.
entities/src/main/java/com/flipkart/varadhi/entities/VaradhiTopic.java (1)
22-41: LGTM!The constructor changes are well-implemented with proper documentation and encapsulation.
server/src/test/java/com/flipkart/varadhi/utils/VaradhiTopicFactoryTest.java (2)
1-29: LGTM! Good improvements to imports and class visibility.The changes follow test class best practices by making imports explicit, using appropriate visibility, and following common conventions for static imports.
44-56: LGTM! Well-structured setup method.The setup method is well organized with clear mock initialization and test data construction.
server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (3)
47-75: LGTM! Good use of Mockito annotations and clear test setup.The test class is well-structured with proper use of
@InjectMocksand@Mockannotations, making the dependencies clear and maintainable.
229-247: LGTM! Comprehensive test coverage for deletion types.The test methods thoroughly cover different deletion scenarios including hard delete, soft delete, default behavior, and invalid input handling.
274-277: LGTM! Clear error response validation.The
assertErrorResponsehelper method provides a clean way to validate error responses.server/src/main/java/com/flipkart/varadhi/web/entities/TopicResource.java (3)
30-31: Consider thread safety implications of mutable state.The
@Setterannotation onactorCodemakes the field mutable despite being marked asfinal. This could lead to thread safety issues if the object is shared across threads.Consider:
- Making the field immutable by removing the
@Setter- Or documenting the thread safety guarantees
33-56: LGTM! Well-documented constructor.The constructor is properly documented and correctly initializes all fields. The private access modifier appropriately enforces the use of factory methods.
58-86: LGTM! Well-implemented factory methods.The factory methods are properly documented and correctly implement the builder pattern. The addition of
actorCodeparameter is consistent with the class's new lifecycle management capabilities.entities/src/test/java/com/flipkart/varadhi/entities/VaradhiTopicTest.java (2)
6-17: LGTM! Well-organized imports and properly named constants.The changes follow Java naming conventions for constants and include all necessary assertions for comprehensive testing.
33-56: LGTM! Comprehensive test coverage with clear assertions.The tests thoroughly validate topic creation with both grouped and non-grouped configurations. The use of
assertAllwith descriptive error messages improves test clarity.server/src/test/java/com/flipkart/varadhi/services/VaradhiTopicServiceTest.java (2)
135-307: Add test for hard deleting a soft-deleted topic.The test suite is missing coverage for hard deleting a topic that's already soft-deleted.
380-470: LGTM! Comprehensive test coverage for topic existence and retrieval.The test methods thoroughly cover topic existence checks, retrieval operations, and filtering of active/inactive topics.
server/src/main/java/com/flipkart/varadhi/utils/VaradhiSubscriptionFactory.java (5)
29-31: LGTM! Good addition of class-level documentation.The added class-level Javadoc clearly describes the purpose of the factory class.
147-148: Address TODO comments regarding region and indexing logic.Two important TODOs need attention:
- Region-specific logic handling
- Indexing of storage topic and subscription names within composite topics/subscriptions
These TODOs are critical for the proper functioning of the system.
Would you like me to help implement solutions for either of these TODOs? I can:
- Implement region-specific logic handling
- Design an indexing mechanism for storage topics and subscriptions
328-329: Good addition of format documentation.The format documentation for internal subscription and topic names is clear and helpful.
Also applies to: 349-350
309-311: Consider future support for multi-shard internal topics.The current implementation throws an exception for multi-shard internal topics. Consider documenting this limitation in the class-level documentation and create a tracking issue for future support.
Would you like me to:
- Update the class-level documentation to mention this limitation?
- Create a tracking issue for implementing multi-shard internal topics support?
74-93: Verify the impact of adding actorCode parameter.The
getmethod has been updated to includeactorCodein theVaradhiSubscriptioncreation, which aligns with the PR's soft deletion feature. However, we should verify that all callers of this method are updated accordingly.✅ Verification successful
I'll run a new shell script to search for all invocations of the
VaradhiSubscriptionFactory.getmethod with the expected three arguments to confirm that callers are not attempting to pass an extra parameter foractorCode.
Impact of adding the actorCode parameter verified
- The only call to
VaradhiSubscriptionFactory.get(inserver/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java:243) supplies three arguments as before.- The factory method internally retrieves
actorCodeviasubscriptionResource.getActorCode(), meaning callers remain unaffected by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to VaradhiSubscriptionFactory.get to ensure they provide actorCode ast-grep --pattern 'varadhiSubscriptionFactory.get($_, $_, $_, $_)'Length of output: 69
Script:
#!/bin/bash # Search for calls to VaradhiSubscriptionFactory.get with three arguments (expected signature) ast-grep --pattern 'varadhiSubscriptionFactory.get($_, $_, $_)'Length of output: 241
server/src/main/java/com/flipkart/varadhi/web/entities/ResourceActionRequest.java (1)
1-21: LGTM! Well-structured record implementation.The record is well-documented and includes appropriate validation for the required field.
server/src/test/java/com/flipkart/varadhi/web/entities/ResourceActionRequestTest.java (1)
1-60: LGTM! Comprehensive test coverage.The test class thoroughly covers all scenarios including constructor validation and getter methods.
server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionTestBase.java (1)
29-174: LGTM! Well-structured refactoring of test utilities.The changes improve code organization and readability through:
- Consistent naming conventions (get -> create/build)
- Clear constant naming with DEFAULT_ prefix
- Enhanced test utilities for generating test data
server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java (2)
162-166: Subscription Retrieval Efficiency IssueThe implementation relies on loading all subscriptions into memory and filtering them, which may lead to performance issues with large datasets.
104-118: Hard delete partial failuresIf
storageTopicService.delete(...)succeeds butmetaStore.deleteTopic(...)fails, resources may become partially deleted. Consider implementing a rollback mechanism.server/src/main/java/com/flipkart/varadhi/web/v1/admin/TopicHandlers.java (2)
198-203: Add validation for restore operation.The
restoremethod should validate the topic's current state before attempting restoration. Consider checking if the topic exists and is in a restorable state.Run the following script to check if there are any state validations in the topic service:
✅ Verification successful
Validation for restore operation is already implemented in the service layer.
TheVaradhiTopicService.restoremethod fetches the topic and checks its state using the conditionif (varadhiTopic.isActive()). If the topic is active (i.e., not deleted), it throws anInvalidOperationForResourceException, ensuring that only topics in a proper state (deleted) can be restored.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for state validation in topic restoration rg -A 10 "restore.*\(.*\)" server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.javaLength of output: 666
288-290: Enhance admin check with proper authorization.The
isVaradhiAdminmethod uses a hardcoded value for admin check. This should be replaced with proper authorization when implemented.server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (1)
476-478: Replace hardcoded admin check.The
isVaradhiAdminmethod uses a hardcoded value. This should be replaced with proper authorization when implemented.
| private TopicCapacityPolicy getShardCapacity(TopicCapacityPolicy topicCapacity, int shardCount) { | ||
| return topicCapacity.from((double) 1 / shardCount, topicCapacity.getReadFanOut()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for shard count.
The getShardCapacity method calculates capacity based on shard count but doesn't validate that the count is positive.
Add input validation:
private TopicCapacityPolicy getShardCapacity(TopicCapacityPolicy topicCapacity, int shardCount) {
+ if (shardCount <= 0) {
+ throw new IllegalArgumentException("Shard count must be positive");
+ }
return topicCapacity.from((double) 1 / shardCount, topicCapacity.getReadFanOut());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private TopicCapacityPolicy getShardCapacity(TopicCapacityPolicy topicCapacity, int shardCount) { | |
| return topicCapacity.from((double) 1 / shardCount, topicCapacity.getReadFanOut()); | |
| } | |
| private TopicCapacityPolicy getShardCapacity(TopicCapacityPolicy topicCapacity, int shardCount) { | |
| if (shardCount <= 0) { | |
| throw new IllegalArgumentException("Shard count must be positive"); | |
| } | |
| return topicCapacity.from((double) 1 / shardCount, topicCapacity.getReadFanOut()); | |
| } |
server/src/main/java/com/flipkart/varadhi/web/entities/SubscriptionResource.java
Show resolved
Hide resolved
server/src/main/java/com/flipkart/varadhi/services/VaradhiTopicService.java
Show resolved
Hide resolved
| public int getIntProperty(String property) { | ||
| return Integer.parseInt(properties.get(property)); | ||
| String value = properties.get(property); | ||
| if (value == null) { | ||
| throw new IllegalArgumentException("Property not found: " + property); | ||
| } | ||
| return Integer.parseInt(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in getIntProperty.
The method should handle NumberFormatException and provide a more descriptive error message.
public int getIntProperty(String property) {
String value = properties.get(property);
if (value == null) {
throw new IllegalArgumentException("Property not found: " + property);
}
- return Integer.parseInt(value);
+ try {
+ return Integer.parseInt(value);
+ } catch (NumberFormatException e) {
+ throw new IllegalArgumentException("Property '" + property + "' value '" + value + "' is not a valid integer");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public int getIntProperty(String property) { | |
| return Integer.parseInt(properties.get(property)); | |
| String value = properties.get(property); | |
| if (value == null) { | |
| throw new IllegalArgumentException("Property not found: " + property); | |
| } | |
| return Integer.parseInt(value); | |
| } | |
| public int getIntProperty(String property) { | |
| String value = properties.get(property); | |
| if (value == null) { | |
| throw new IllegalArgumentException("Property not found: " + property); | |
| } | |
| try { | |
| return Integer.parseInt(value); | |
| } catch (NumberFormatException e) { | |
| throw new IllegalArgumentException("Property '" + property + "' value '" + value + "' is not a valid integer"); | |
| } | |
| } |
server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java
Show resolved
Hide resolved
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java
Show resolved
Hide resolved
- Fixed issues in existing end-to-end (E2E) tests to ensure they pass successfully. - Addressed PR review comments by making necessary code adjustments and improvements.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (1)
374-378: 🛠️ Refactor suggestionAdd transaction support for soft deletion.
The soft deletion process should be atomic to prevent inconsistencies.
private void handleSoftDelete(VaradhiSubscription subscription, ResourceActionRequest actionRequest) { - subscription.markInactive(actionRequest.actorCode(), actionRequest.message()); - metaStore.updateSubscription(subscription); - log.info("Subscription '{}' marked inactive successfully.", subscription.getName()); + try { + metaStore.beginTransaction(); + subscription.markInactive(actionRequest.actorCode(), actionRequest.message()); + metaStore.updateSubscription(subscription); + metaStore.commitTransaction(); + log.info("Subscription '{}' marked inactive successfully.", subscription.getName()); + } catch (Exception e) { + metaStore.rollbackTransaction(); + log.error("Failed to soft delete subscription '{}'.", subscription.getName(), e); + throw e; + } }
🧹 Nitpick comments (3)
server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java (1)
249-264: Add test cases for subscription restoration edge cases.Consider adding test cases for:
- Attempting to restore an already active subscription
- Attempting to restore with insufficient permissions
- Attempting to restore a hard-deleted subscription
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (1)
256-271: Consider optimizing update operation.The update operation has two areas for improvement:
- The TODO suggests splitting into separate APIs for different update types
- Updates are allowed even when there are no changes to the subscription
Consider adding change detection before proceeding with updates.
public void update(RoutingContext ctx) { SubscriptionResource subscription = getValidSubscriptionResource(ctx); + VaradhiSubscription existingSubscription = subscriptionService.getSubscription( + subscription.getSubscriptionInternalName()); + + if (!hasChanges(existingSubscription, subscription)) { + ctx.endApiWithResponse(SubscriptionResource.from(existingSubscription)); + return; + } + ctx.handleResponse( subscriptionService.updateSubscription( subscription.getSubscriptionInternalName(),server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (1)
301-368: Add tests for concurrent deletion scenarios.While the deletion test coverage is good, consider adding tests for:
- Concurrent deletion attempts
- Deletion during other operations (start/stop)
- Race conditions between soft and hard deletion
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java(1 hunks)server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java(2 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/DlqHandlers.java(1 hunks)server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java(3 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java(2 hunks)server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java(2 hunks)server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
server/src/main/java/com/flipkart/varadhi/web/v1/admin/DlqHandlers.java (1)
80-81: Verify the intended behavior with soft-deleted subscriptions.The change from
getSubscriptiontogetSubscriptionWithoutValidationbypasses state validation. While this might be intentional for handling soft-deleted resources, it creates an inconsistency with other methods in the class that still usegetSubscription.Please confirm:
- Should soft-deleted subscriptions be accessible through the DLQ API?
- If yes, shouldn't
enqueueUnsideline(line 93) andgetMessages(line 100) also usegetSubscriptionWithoutValidation?Run this script to analyze the validation behavior across the codebase:
entities/src/main/java/com/flipkart/varadhi/entities/ResourceDeletionType.java (1)
1-27: LGTM! Well-structured enum with clear documentation.The implementation is clean, well-documented, and handles edge cases appropriately by defaulting to
SOFT_DELETEfor invalid values.server/src/testE2E/java/com/flipkart/varadhi/SubscriptionTests.java (1)
222-248: LGTM! Comprehensive test coverage for subscription restoration.The test method thoroughly validates:
- Restoration of soft-deleted subscriptions
- State validation before restoration
- Proper error handling
server/src/test/java/com/flipkart/varadhi/web/admin/TopicHandlersTest.java (2)
229-247: LGTM! Good test coverage for different deletion types.The test methods thoroughly validate:
- Hard deletion
- Soft deletion
- Default behavior
- Invalid deletion type handling
249-257: Add test cases for topic restoration edge cases.Consider adding test cases for:
- Attempting to restore a non-existent topic
- Attempting to restore a topic that wasn't soft-deleted
- Attempting to restore a hard-deleted topic
server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java (1)
349-366: 🛠️ Refactor suggestionAdd transaction support for hard deletion.
The hard deletion process should be atomic to prevent inconsistencies if
deProvisionfails.private void handleHardDelete( VaradhiSubscription subscription, Project subProject, ResourceActionRequest actionRequest ) { subscription.markDeleting(actionRequest.actorCode(), actionRequest.message()); - metaStore.updateSubscription(subscription); + try { + metaStore.beginTransaction(); + metaStore.updateSubscription(subscription); - try { shardProvisioner.deProvision(subscription, subProject); metaStore.deleteSubscription(subscription.getName()); + metaStore.commitTransaction(); log.info("Subscription '{}' deleted successfully.", subscription.getName()); } catch (Exception e) { log.error("Failed to hard delete subscription '{}'.", subscription.getName(), e); subscription.markDeleteFailed(e.getMessage()); - metaStore.updateSubscription(subscription); + metaStore.rollbackTransaction(); throw e; } }Likely invalid or redundant comment.
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java (2)
1-93: LGTM! Well-structured class setup.The class organization has been improved with proper imports, documentation, and initialization. The static final constant for retries is a good practice.
301-307: Add validation for restore operation.The
restoremethod should validate the subscription's current state before attempting restoration.server/src/test/java/com/flipkart/varadhi/web/admin/SubscriptionHandlersTest.java (2)
45-114: LGTM! Well-structured test setup.The test class is well-organized with proper base class extension, mock setup, and route configuration.
500-526: Enhance lifecycle operation test coverage.The start/stop operation tests only cover success cases. Additional test cases are needed for error scenarios.
server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java
Show resolved
Hide resolved
| @@ -34,28 +56,39 @@ private VaradhiSubscription( | |||
| RetryPolicy retryPolicy, | |||
| ConsumptionPolicy consumptionPolicy, | |||
| SubscriptionShards shards, | |||
| Status status, | |||
| LifecycleStatus status, | |||
There was a problem hiding this comment.
entities with lifecycle can probably become a base class. so Topic & Subscription can extend that. and methods to change the status can move to that base class.
|
|
||
| testE2E { | ||
| useJUnitPlatform() | ||
| jvmArgs '--add-opens', 'java.base/java.net=ALL-UNNAMED' |
There was a problem hiding this comment.
add-opens shouldn't be required.
Description [#215]
This PR introduces support for soft and hard deletion for Varadhi resources, enabling flexible lifecycle management for topics and subscriptions. It includes updates to the service layer, deletion handler, restoration process, and unit tests to ensure a robust and maintainable implementation.
Key Changes
Here are the release notes:
New Features
LifecycleStatusclass to encapsulate lifecycle states and action codes.ResourceActionRequestrecord for action requests on resources.Improvements
SubscriptionResourceandTopicResourceto include actor codes.Bug Fixes
Technical Enhancements
Type of Change
Testing and Validation
Added comprehensive unit tests to verify the correctness of the implementation.
Included edge cases to ensure robust behavior, such as:
Refactored test code for improved maintainability and readability.
Checklist:
Summary by CodeRabbit
New Features
ResourceActionRequestrecord to encapsulate action requests with actor codes and messages.Enhancements
VaradhiSubscriptionFactoryandVaradhiTopicFactorywith improved methods for shard management and topic creation.