S3 ChangeLog store with filtered domain support#3240
S3 ChangeLog store with filtered domain support#3240havetisyan wants to merge 1 commit intomasterfrom
Conversation
Signed-off-by: Henry Avetisyan <hga@yahooinc.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new S3 ChangeLog store implementation that supports filtering domains. This enhancement allows ZTS instances to be configured to only process and load data for a specific subset of domains from S3, which is particularly useful for deployments focused on user certificate issuance, reducing load and resource consumption by avoiding the need to monitor and load all domain data. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a filtered version of the S3ChangeLogStore, allowing domain lists to be filtered based on a configured set of supported domains. This is achieved by creating a new S3FilteredChangeLogStore and a corresponding S3FilteredChangeLogStoreFactory. The base S3ChangeLogStore is updated to support this extension by changing member visibility and adding an overloaded method for filtering.
The overall approach is sound and the new functionality is well-tested. I have a couple of suggestions for improvement: one to remove redundant initialization code in the new S3FilteredChangeLogStore, and another to make the parsing of the filter property in S3FilteredChangeLogStoreFactory more robust.
| super.init(); | ||
| super.initAwsRegion(); |
There was a problem hiding this comment.
The calls to super.init() and super.initAwsRegion() are redundant. The superclass's default constructor, which is implicitly called as the first operation in this constructor, already performs this initialization. Removing these lines will prevent the initialization logic from executing twice.
| for (String domain : filter.split(",")) { | ||
| domains.add(domain.trim()); | ||
| } |
There was a problem hiding this comment.
The current logic for parsing the domain filter property is not robust against empty or whitespace-only values (e.g., "", " ", ","). This can result in a Set containing an empty string, which is likely not intended and would not be caught by the domains.isEmpty() check on line 61. It's better to filter out empty strings while populating the set.
for (String domain : filter.split(",")) {
String trimmedDomain = domain.trim();
if (!trimmedDomain.isEmpty()) {
domains.add(trimmedDomain);
}
}1 participant
Description
can be used with #3238 to have an ZTS instance allocated for user certs only
Contribution Checklist:
Attach Screenshots (Optional)