-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ADRs: 017-019 - Adding Impact Sections #1628
ADRs: 017-019 - Adding Impact Sections #1628
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
||
### Positive | ||
|
||
- **High Performance:** HikariCP outperforms other connection pool libraries in terms of throughput and latency, enhancing application responsiveness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding specific examples or metrics to support claims about performance and scalability in the Impact section. This would provide clearer insights into the benefits of using HikariCP for connection pooling. [important]
|
||
### Risks | ||
|
||
- **Migration Errors:** Incorrectly defined migration scripts could cause deployment failures or data integrity issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be beneficial to include a subsection under Risks about potential security concerns with migration scripts, such as exposure of sensitive data or SQL injection risks, and how to mitigate them. [important]
|
||
### Negative | ||
|
||
- **Increased Complexity:** Adds another table to the database, which introduces additional queries and joins in the codebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To enhance the Negative section, consider discussing the potential increase in maintenance costs associated with managing an additional table, especially in terms of database administration and backup complexities. [medium]
PR Code Suggestions ✨No code suggestions found for the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added two comments to consider on risk sections of 018 and 019, but otherwise looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides my nitpick and Gilmore's comments, LGTM!
- **Operational Challenges:** Monitoring and maintaining the pool in production environments may require additional effort. | ||
|
||
|
||
- **Dependency Lock-in:** Switching to another connection pooling library in the future may require significant refactoring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should our use of OEA reduce this to limited refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't including mitigations for these ADRs, but if we did, I would definitely be including this answer. I think the fact that we recognize (and document) that we are aware of this risk resolves this story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
This reverts commit 7728b0f.
- Changelog Structure and Dependencies
- added suggested risk language in regards to adding another table increases potential rollback complexity
Quality Gate passedIssues Measures |
Description
This PR covers filling in the impact sections for ADRs 017-019 so that it is understood why the decision was made at the time and what the concerns were.
This PR errs on the side of being more verbose than not so that unneeded items can be stripped away during review, so please read carefully.
Issue #1247
Checklist