LNK-3007: Testing/Integration Testing - select/store data based on fa… #594
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces facility-specific enhancements to Kafka consumer management across multiple components. The changes modify the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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: 1
🧹 Nitpick comments (4)
DotNet/LinkAdmin.BFF/Application/Commands/Integration/KafkaConsumerService.cs (2)
72-75: Storing updated correlation IDs in Redis
Storing the updated list back into Redis for each consumed message is a sensible approach. However, consider adding logging or instrumentation in case of Redis failures or slowdowns to aid troubleshooting.
95-123: Robust facility extraction logic
This method gracefully handles both valid JSON and plain string cases. As an enhancement, consider logging JSON parsing errors for better observability when malformed keys are encountered.DotNet/LinkAdmin.BFF/Application/Commands/Integration/KafkaConsumerManager.cs (1)
60-77: ClearRedisCache: Potential logging improvements
Clearing all facility-specific keys is correct for a fresh start. Consider logging which keys have been removed to aid in debugging.DotNet/LinkAdmin.BFF/Presentation/Endpoints/IntegrationTestingEndpoints.cs (1)
131-133: CreateConsumersRequested
Explicitly passingfacility.FacilityIdclarifies scope. Consider returning an explicit response or success code to indicate that consumers have started successfully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
DotNet/LinkAdmin.BFF/Application/Commands/Integration/KafkaConsumerManager.cs(2 hunks)DotNet/LinkAdmin.BFF/Application/Commands/Integration/KafkaConsumerService.cs(4 hunks)DotNet/LinkAdmin.BFF/Application/Models/Integration/Facility.cs(1 hunks)DotNet/LinkAdmin.BFF/Presentation/Endpoints/IntegrationTestingEndpoints.cs(4 hunks)
🔇 Additional comments (12)
DotNet/LinkAdmin.BFF/Application/Commands/Integration/KafkaConsumerService.cs (3)
6-7: New references for JSON and Regex
The addition of Newtonsoft.Json.Linq and System.Text.RegularExpressions looks appropriate for parsing Kafka message keys and filtering by facility.
27-27: Added ‘facility’ parameter to StartConsumer
Including a facility parameter is a good approach to distinguish or skip messages from non-target facilities. Ensure that all invocations of this method are updated to supply the correct facility to avoid consumer mismatches.
48-59: Facility-based message filtering and Redis key handling
Skipping messages that don’t match the target facility appears correct. Confirm the approach meets functional requirements (e.g., do we need to log or handle these skipped messages differently?). Also verify concurrency scenarios for Redis access during high throughput.
DotNet/LinkAdmin.BFF/Application/Commands/Integration/KafkaConsumerManager.cs (4)
79-89: CreateAllConsumers: Redis clearing prior to consumer creation
It’s logical to clear cached correlation IDs before starting new consumers. Confirm that you really want to discard prior correlation data, as this might affect historical tracking.
Line range hint 94-117: CreateConsumer: Building IConsumer with facility
Switching to IConsumer<string, string> is consistent with the new facility-based message key usage. Exception handling within Task.Run(not shown) appears adequate, but consider whether additional fault handling or logging is required.
121-134: readAllConsumers: Retrieving correlation IDs from Redis
Reading correlation IDs per topic is clear. If a Redis value is null or missing, ensure no unexpected exceptions occur. Also consider a fallback to an empty list or a default placeholder to better handle missing data.
139-152: StopAllConsumers: Cleaning up concurrency
Stopping all consumers and clearing Redis is an effective reset. However, verify if historical correlation IDs are needed post-stop for troubleshooting. If so, consider persisting them elsewhere.
DotNet/LinkAdmin.BFF/Application/Models/Integration/Facility.cs (1)
1-14: New Facility model
Defining a simple FacilityId property is a clean approach. For future enhancements, consider adding validations or constraints (e.g., non-null, length limitations) to prevent invalid facility identifiers.
DotNet/LinkAdmin.BFF/Presentation/Endpoints/IntegrationTestingEndpoints.cs (4)
93-95: POST mapping to start consumers
Switching to a POST endpoint and including facility context is consistent with the new design. Verify that clients know they must provide a Facility payload in the request body.
104-104: POST mapping to read consumers
Using POST for retrieving consumer states is unconventional but can be valid if an input payload is needed. Ensure this aligns with your API design standards.
115-115: POST mapping to stop consumers
Uniformly using POST is coherent. Confirm that correlation or usage data isn’t prematurely wiped if multiple tests run concurrently.
137-139: ReadConsumersRequested
Reading Redis for the specified facility ensures isolation per facility. As a safeguard, consider handling empty or null FacilityId strings.
DotNet/LinkAdmin.BFF/Presentation/Endpoints/IntegrationTestingEndpoints.cs
Outdated
Show resolved
Hide resolved
…egrationTest' into user/ariana/LNK-3007-Testing/IntegrationTest
arianamihailescu
deleted the
user/ariana/LNK-3007-Testing/IntegrationTest
branch
…cility
🛠️ Description of Changes
Please provide a high-level overview of the changes included in this PR.
🧪 Testing Performed
Please describe the testing that was performed on the changes included in this PR.
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
New Features
Facilitymodel to support facility identification.Improvements
Changes