Review and tidy solr/modules/s3-repository code

solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepositoryConfig.java

@@ -67,25 +67,6 @@ static String getStringConfig(NamedList<?> config, String property) {
     }
   }
 
-  static int getIntConfig(NamedList<?> config, String property) {
-    return getIntConfig(config, property, 0);
-  }
-
-  static int getIntConfig(NamedList<?> config, String property, int def) {
-    String envProp = EnvUtils.getProperty(property);
-    if (envProp == null) {
-      Object configProp = config.get(property);
-      return configProp instanceof Integer ? (int) configProp : def;
-    } else {
-      return Integer.parseInt(envProp);
-    }
-  }
-
-  /** If the property as any other value than 'true' or 'TRUE', this will default to false. */
-  static boolean getBooleanConfig(NamedList<?> config, String property) {
-    return getBooleanConfig(config, property, false);
-  }
-
   static boolean getBooleanConfig(NamedList<?> config, String property, boolean def) {
     String envProp = EnvUtils.getProperty(property);
     if (envProp == null) {

solr/modules/s3-repository/src/java/org/apache/solr/s3/S3OutputStream.java

@@ -151,7 +151,7 @@ public void flush() throws IOException {
     }
 
     // Flush is possible only if we have more data than the required part size
-    // If buffer size is lower than than, just skip
+    // If buffer size is lower, then just skip
     if (buffer.position() - buffer.arrayOffset() >= MIN_PART_SIZE) {
       uploadPart();
     }

solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java

@@ -230,7 +230,7 @@ void delete(Collection<String> paths) throws S3Exception {
   }
 
   /**
-   * Delete directory, all the files and sub-directories from S3.
+   * Delete directory, all the files and subdirectories from S3.
    *
    * @param path Path to directory in S3.
    */
@@ -247,10 +247,10 @@ void deleteDirectory(String path) throws S3Exception {
   }
 
   /**
-   * List all the files and sub-directories directly under given path.
+   * List all the files and subdirectories directly under given path.
    *
    * @param path Path to directory in S3.
-   * @return Files and sub-directories in path.
+   * @return Files and subdirectories in path.
    */
   String[] listDir(String path) throws S3Exception {
     path = sanitizedDirPath(path);
@@ -432,7 +432,7 @@ private Collection<String> deleteObjects(Collection<String> paths) throws S3Exce
        * Per the S3 docs:
        * https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/model/DeleteObjectsResult.html
        * An exception is thrown if there's a client error processing the request or in S3 itself.
-       * However, there's no guarantee the delete did not happen if an exception is thrown.
+       * However, there's no guarantee the delete operation did not happen if an exception is thrown.
        */
       return deleteObjects(paths, MAX_KEYS_PER_BATCH_DELETE);
     } catch (SdkException sdke) {
@@ -548,7 +548,7 @@ private String getParentDirectory(String path) {
   }
 
   /** Ensures path adheres to some rules: -Doesn't start with a leading slash */
-  String sanitizedPath(String path) throws S3Exception {
+  String sanitizedPath(String path) {
     // Trim space from start and end
     String sanitizedPath = path.trim();
 
@@ -584,7 +584,7 @@ String sanitizedFilePath(String path) throws S3Exception {
    * Ensures directory path adheres to some rules: -Overall Path rules from `sanitizedPath` -Add a
    * trailing slash if one does not exist
    */
-  String sanitizedDirPath(String path) throws S3Exception {
+  String sanitizedDirPath(String path) {
     // Trim space from start and end
     String sanitizedPath = sanitizedPath(path);
 

solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java

@@ -120,18 +120,18 @@ public void testLocalDirectoryFunctions() throws Exception {
       repo.createDirectory(path);
       assertTrue(repo.exists(path));
       assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(path));
-      assertEquals("No files should exist in dir yet", repo.listAll(path).length, 0);
+      assertEquals("No files should exist in dir yet", 0, repo.listAll(path).length);
 
       URI subDir = new URI("/test/dir/");
       repo.createDirectory(subDir);
       assertTrue(repo.exists(subDir));
       assertEquals(BackupRepository.PathType.DIRECTORY, repo.getPathType(subDir));
-      assertEquals("No files should exist in subdir yet", repo.listAll(subDir).length, 0);
+      assertEquals("No files should exist in subdir yet", 0, repo.listAll(subDir).length);
 
       assertEquals(
           "subDir should now be returned when listing all in parent dir",
-          repo.listAll(path).length,
-          1);
+          1,
+          repo.listAll(path).length);
 
       repo.deleteDirectory(path);
       assertFalse(repo.exists(path));
@@ -258,7 +258,7 @@ public void testRandomAccessInput() throws Exception {
    * Check implementation of {@link S3BackupRepository#openInput(URI, String, IOContext)}. Open an
    * index input and seek to an absolute position.
    *
-   * <p>We use specified text. It must has the word "content" at given position.
+   * <p>We use specified text. It must have the word "content" at given position.
    */
   private void doRandomAccessTest(String content, int position) throws Exception {
 
@@ -304,7 +304,7 @@ public void testBackwardRandomAccess() throws Exception {
       input.readBytes(buffer, 0, BufferedIndexInput.BUFFER_SIZE * 2);
 
       // Seek back to the 5th byte.
-      // It is not any more in the internal buffer, so we should fail
+      // It is not anymore in the internal buffer, so we should fail
       IOException exception = assertThrows(IOException.class, () -> input.seek(5));
       assertEquals("Cannot seek backward", exception.getMessage());
     }

solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java

@@ -19,16 +19,13 @@
 
 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters;
 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakLingering;
-import java.lang.invoke.MethodHandles;
 import org.apache.lucene.tests.util.LuceneTestCase;
 import org.apache.lucene.tests.util.QuickPatchThreadsFilter;
 import org.apache.solr.SolrIgnoredThreadsFilter;
 import org.apache.solr.cloud.api.collections.AbstractIncrementalBackupTest;
 import org.apache.solr.util.LogLevel;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import software.amazon.awssdk.regions.Region;
 
 // Backups do checksum validation against a footer value not present in 'SimpleText'
@@ -45,7 +42,6 @@
     value =
         "org.apache.solr.cloud=DEBUG;org.apache.solr.cloud.api.collections=DEBUG;org.apache.solr.cloud.overseer=DEBUG")
 public class S3IncrementalBackupTest extends AbstractIncrementalBackupTest {
-  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
   private static final String BUCKET_NAME = S3IncrementalBackupTest.class.getSimpleName();
 
@@ -93,8 +89,6 @@ public class S3IncrementalBackupTest extends AbstractIncrementalBackupTest {
           + "  \n"
           + "</solr>\n";
 
-  private static String backupLocation;
-
   @BeforeClass
   public static void ensureCompatibleLocale() {
     // TODO: Find incompatible locales

solr/modules/s3-repository/src/test/org/apache/solr/s3/S3OutputStreamTest.java

@@ -102,7 +102,7 @@ public void testWriteSmallBuffer() throws IOException {
     }
   }
 
-  /** Write a byte array larger than S3 part size. Simulate a real multi-part upload. */
+  /** Write a byte array larger than S3 part size. Simulate a real multipart upload. */
   @Test
   public void testWriteLargeBuffer() throws IOException {
     // must be larger than S3 part size

solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java

@@ -99,7 +99,7 @@ public void testDirectoryLength() throws Exception {
     assertThat(exception.getMessage(), exception.getMessage(), containsString("Path is Directory"));
   }
 
-  /** Check various method throws the expected exception of a missing S3 key. */
+  /** Check various methods throw the expected exception for a missing S3 key. */
   @Test
   public void testNotFound() {
     assertThrows(S3NotFoundException.class, () -> client.pullStream("/not-found"));
@@ -166,7 +166,7 @@ public void testReadWithConnectionLoss() throws IOException {
               break;
           }
           // Initiate a connection loss at the beginning of every "bytesPerException" cycle.
-          // The input stream will not immediately see an error, it will have pre-loaded some data.
+          // The input stream will not immediately see an error, it will have preloaded some data.
           if ((byteCount % bytesPerException <= maxBuffer)) {
             initiateS3ConnectionLoss();
           }