You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by ds...@apache.org on 2026/09/20 19:19:25 UTC
(solr) 09/12: Review and tidy solr/modules/s3-repository code (#4838)
This is an automated email from the ASF dual-hosted git repository.
dsmiley pushed a commit to branch branch_10x
in repository https://gitbox.apache.org/repos/asf/solr.git
commit ff8983b552b5e4212ce20969aeeeda3bc06192f4
Author: Eric Pugh <ep...@opensourceconnections.com>
AuthorDate: Thu Sep 17 14:15:25 2026 -0400
Review and tidy solr/modules/s3-repository code (#4838)
(cherry picked from commit 82fccd0809255bbbbb64b1fffc9abd7cb353fabd)
---
.../org/apache/solr/s3/S3BackupRepositoryConfig.java | 19 -------------------
.../src/java/org/apache/solr/s3/S3OutputStream.java | 2 +-
.../src/java/org/apache/solr/s3/S3StorageClient.java | 12 ++++++------
.../org/apache/solr/s3/S3BackupRepositoryTest.java | 12 ++++++------
.../org/apache/solr/s3/S3IncrementalBackupTest.java | 6 ------
.../test/org/apache/solr/s3/S3OutputStreamTest.java | 2 +-
.../src/test/org/apache/solr/s3/S3ReadWriteTest.java | 4 ++--
7 files changed, 16 insertions(+), 41 deletions(-)
diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepositoryConfig.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepositoryConfig.java
index 43d6279bc94..06cc54f6795 100644
--- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepositoryConfig.java
+++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepositoryConfig.java
@@ -67,25 +67,6 @@ public class S3BackupRepositoryConfig {
}
}
- 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) {
diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3OutputStream.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3OutputStream.java
index 25bf3465e7f..3f46905fa49 100644
--- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3OutputStream.java
+++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3OutputStream.java
@@ -151,7 +151,7 @@ public class S3OutputStream extends OutputStream {
}
// 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();
}
diff --git a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java
index 6124d98e812..bf076226685 100644
--- a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java
+++ b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java
@@ -230,7 +230,7 @@ public class S3StorageClient {
}
/**
- * 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 @@ public class S3StorageClient {
}
/**
- * 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 @@ public class S3StorageClient {
* 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 @@ public class S3StorageClient {
}
/** 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 @@ public class S3StorageClient {
* 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);
diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
index 5d4f6c3af54..bd24ac0a7cc 100644
--- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
+++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
@@ -120,18 +120,18 @@ public class S3BackupRepositoryTest extends AbstractBackupRepositoryTest {
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 class S3BackupRepositoryTest extends AbstractBackupRepositoryTest {
* 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 class S3BackupRepositoryTest extends AbstractBackupRepositoryTest {
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());
}
diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java
index f20d3f89cac..9546012190b 100644
--- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java
+++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java
@@ -19,7 +19,6 @@ package org.apache.solr.s3;
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;
@@ -27,8 +26,6 @@ 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 @@ import software.amazon.awssdk.regions.Region;
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
diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3OutputStreamTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3OutputStreamTest.java
index 4df12150956..df1ae9991eb 100644
--- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3OutputStreamTest.java
+++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3OutputStreamTest.java
@@ -103,7 +103,7 @@ public class S3OutputStreamTest extends SolrTestCaseJ4 {
}
}
- /** 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
diff --git a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java
index 9f360c644ac..3574587b4fa 100644
--- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java
+++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java
@@ -99,7 +99,7 @@ public class S3ReadWriteTest extends AbstractS3ClientTest {
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 class S3ReadWriteTest extends AbstractS3ClientTest {
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();
}