prateeksinghalgit commented on PR #3750: URL: https://github.com/apache/solr/pull/3750#issuecomment-4428454450
> Only reviewed the production code (skiped tests and other stuff). > > Some general comments on the review: > > * can you replace all unnecessary qualified class names by imports? (replacing `java.lang.String` by `String`) This makes the code painful to read > * 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 > * we always upload files with a _block list_. At least for small files, can we keep things simples with a simple pull? It think we may skip the commit call. > * 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). > > Please note I did a code review, but I have no way to test it with Azure. @psalagnac , thank you for taking the time to review. 1) made the change by replacing all unnecessary qualified class names by imports 2) 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) 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]
