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
1620226: Perform CRL generation without exceeding memory and maxing CPU #2204
Conversation
retest this please |
*/ | ||
public CandlepinQuery<Long> getUncollectedRevokedCertSerials() { | ||
DetachedCriteria criteria = DetachedCriteria.forClass(CertificateSerial.class) | ||
.add(Restrictions.gt("expiration", getExpiryRestriction())) |
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.
👍
public boolean syncCRLWithDB(File file) throws IOException { | ||
List<Long> uncollected = this.certificateSerialCurator.getUncollectedRevokedCertSerials().list(); | ||
List<BigInteger> revoke = new ArrayList<>(uncollected.size()); | ||
public int batchSyncCRLWithDB(File crlFile, int batchSize) throws IOException { |
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.
Given how we use the batchSize, is it something we want to do once here rather than implicitly forcing the configured value to be looked up by the two callers?
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.
I'm Ok with either. I was just trying to avoid injecting the candlepin config into the curator. No big deal if you'd like to see this changed.
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.
Up to you, given discussions around this class.
For what it's worth, I see the config lookup being done here internally to the CrlFileUtil rather than the curator. But if the curator itself ended up needing it, it's already injected into every curator through the AbstractHibernatCurator; so we're good there.
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.
Fair points, I'll move it into the CrlFileUtil.
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.
DONE. Once you give the changes a once over I'll squash the update commit before it is merged.
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.
Changes look good
This is good to be merged, but I want to give a chance to respond to the comments above regarding the location of the config reading. |
c885626
to
4757bce
Compare
Changes look good. Squash away when you have time. |
Using a HashSet instead of an ArrayList to store the serials improves the performance of .contains when checking if an entry in the CRL file should be removed due to expiry. Batch lookup/process fetched serials to keep the memory footprint down and to allow garbage collection of the serials earlier. The number of records per batch can be configured via the candlepin config file. NOTE: Due to the way the CRL generation framework works, when batching the serial processing, we end up re-scanning the crl num_serials/batch_size times. This isn't ideal, but we will make due until we make the move to generating a plain text serial file.
4757bce
to
4f3a72f
Compare
DONE |
Using a HashSet instead of an ArrayList to store the serials improves
the performance of .contains when checking if an entry in the CRL file
should be removed due to expiry.
Batch lookup/process fetched serials to keep the memory footprint down
and to allow garbage collection of the serials earlier. The number of
records per batch can be configured via the candlepin config file.
NOTE: Due to the way the CRL generation framework works, when batching
the serial processing, we end up re-scanning the crl num_serials/batch_size times.
This isn't ideal, but we will make due until we make the move to generating
a plain text serial file.
Testing
top | grep tomcat