Conversation
Reviewer's Guide by SourceryThis pull request augments the application for deployment by introducing Terraform configurations for AWS infrastructure, updating the application code for cloud readiness, enhancing event processing logic with batching and "one-at-a-time" execution, improving database replication slot management, and updating the build process to use the shadowJar plugin. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @p14n - I've reviewed your changes - here's some feedback:
Overall Comments:
- This PR mixes application code changes with significant infrastructure additions (Terraform); consider splitting these into separate PRs for easier review.
- The gRPC client now uses TLS but skips certificate verification; ensure this is acceptable for the target deployment environment security requirements.
- The main application loop in
App.javaappears to have changed significantly with the commenting out ofwriteContinuously; ensure this reflects the intended runtime behavior.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| public interface OneAtATimeBehaviour { |
There was a problem hiding this comment.
issue (bug_risk): Revisit shared state in OneAtATimeBehaviour.
Defining final AtomicInteger and AtomicBoolean in the interface means these fields are shared across all implementers. This could lead to unexpected interactions when multiple instances use the oneAtATime behavior concurrently. Consider moving these to instance variables in an abstract class or as part of the concrete implementations.
| @Test | ||
| void shouldClearOldSlots() throws SQLException { | ||
| databaseSetup.clearOldSlots(); |
There was a problem hiding this comment.
suggestion (testing): Test for clearOldSlots should verify behavior more thoroughly.
While it's good that a test was added for the new clearOldSlots method, this test only calls the method without asserting its effects. Consider enhancing this test to:
- Create one or two inactive replication slots with the expected naming pattern (
postevent%) before callingclearOldSlots. - Verify that these specific slots are deleted after the call.
- Verify that active slots or slots with different names are not deleted.
- Test the case where no matching inactive slots exist.
Summary by Sourcery
Introduce AWS deployment infrastructure using Terraform and adapt the application for cloud deployment, including TLS encryption and build process updates.
Enhancements:
OneAtATimeBehaviour.Build:
postgresqlandgrpc-netty-shadeddependencies.Deployment:
make-image.sh) for building the application uber JAR and pushing the corresponding Docker image to ECR.Tests:
DeterministicConsumerTest).Chores: