prateeksinghalgit commented on PR #3750: URL: https://github.com/apache/solr/pull/3750#issuecomment-4428441333
@psalagnac , thank you for taking the time to review. 1. can you replace all unnecessary qualified class names by imports? made the change by replacing all unnecessary qualified class names by imports 2. there is a lot of shared logic between Azure backups and S3 backups. Lot of code is duplicated. I wonder if abstract classes should be introduced to factor out some logic? - s3-repository is in active production use; a refactor would force shared abstractions and re-testing on a stable module to land a brand-new one. That's significant scope creep with no end-user benefit. Also the duplication is mostly in boilerplate (resolveDirectory shape, sanitization helpers); both repositories have different SDK contracts underneath. Let me know what you think. 3. overall, I think it's pretty inefficient that AzureBlobIndexInput always reads files by small pages. Mostly for backup/restore, we will only fetch full files. You should not do a request by page, or use much bigger pages (without keeping them in memory) ? Given the comments on AzureBlobIndexInput, I have rewritten AzureBlobIndexInput to extend Lucene's standard BufferedIndexInput instead of IndexInput directly. Now only override readInternal(ByteBuffer) and seekInternal(long), and everything else (buffer management, slice, clone, EOF handling) comes from the parent class. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
