dsmiley commented on a change in pull request #108:
URL: https://github.com/apache/solr/pull/108#discussion_r651425759
##########
File path:
solr/contrib/blob-directory/src/java/org/apache/solr/blob/BlobDirectory.java
##########
@@ -61,20 +62,33 @@
private final Object lock = new Object();
private volatile boolean isOpen;
- public BlobDirectory(Directory delegate, String blobDirPath,
BlobStoreConnection blobStoreConnection) throws IOException {
+ public BlobDirectory(Directory delegate, String blobDirPath, BlobRepository
blobRepository) throws IOException {
super(delegate);
this.blobDirPath = blobDirPath;
- this.blobStoreConnection = blobStoreConnection;
+ this.blobRepository = blobRepository;
pullMissingFilesFromRepo();
}
private void pullMissingFilesFromRepo() throws IOException {
Set<String> localFileNames = new HashSet<>(Arrays.asList(in.listAll()));
Review comment:
blobRepository.pull might not be internally multi-threaded (?) but I
could easily imagine we might consider making it so. I think we should either
make localFileNames & numPulledFiles thread-safe (or use explicit
synchronization; probably simpler?) OR add a comment to recognize we don't need
to worry about thread-safety at this time. This way if it happens, we'll be
less likely to make a mistake then.
##########
File path:
solr/contrib/blob-directory/src/test/org/apache/solr/blob/BlobDirectoryTest.java
##########
@@ -172,7 +174,8 @@ public void testSyncFromRepository() throws IOException {
// When
// - The local files are wiped (e.g. host crash).
- deleteRecursively(new File(directoryPath));
+ MoreFiles.deleteRecursively(Path.of(directoryPath),
RecursiveDeleteOption.ALLOW_INSECURE);
+ assertFalse(Files.exists(Path.of(directoryPath)));
Review comment:
LOL are we testing Guava here ;-). Not needed but whatever.
##########
File path:
solr/contrib/blob-directory/src/java/org/apache/solr/blob/BlobDirectory.java
##########
@@ -61,20 +62,33 @@
private final Object lock = new Object();
private volatile boolean isOpen;
- public BlobDirectory(Directory delegate, String blobDirPath,
BlobStoreConnection blobStoreConnection) throws IOException {
+ public BlobDirectory(Directory delegate, String blobDirPath, BlobRepository
blobRepository) throws IOException {
super(delegate);
this.blobDirPath = blobDirPath;
- this.blobStoreConnection = blobStoreConnection;
+ this.blobRepository = blobRepository;
pullMissingFilesFromRepo();
}
private void pullMissingFilesFromRepo() throws IOException {
Set<String> localFileNames = new HashSet<>(Arrays.asList(in.listAll()));
- blobStoreConnection.pull(blobDirPath, this::openOutput, blobFile -> {
- //TODO: We could also check the size and checksum.
- return !localFileNames.remove(blobFile.fileName());
+ MutableInt numPulledFiles = log.isInfoEnabled() ? new MutableInt() : null;
Review comment:
Is the only reason you want to use MutableInt vs a primitive integer
because MutableInt can be null? If so, I think it's equally clear to have the
log.isInfoEnabled check later and simply increment the numPulledFiles always.
I'd say _clearer_ because then the log.info call would be guarded by a clear
"log.isInfoEnabled" that Solr's source code checks demand to see and which many
people come to expect any way.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
