From 144397416cb28b8be6a7c7cfad45858bffe59dfc Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Mon, 31 Aug 2026 13:22:57 -0400 Subject: [PATCH] Review and tidy solr/modules/gcs-repository code Split out from #4743 into a per-module PR to make review easier. Contains only the tidy-up changes to solr/modules/gcs-repository (unused fields/params, javadoc/comment fixes). No behavior changes. --- .../org/apache/solr/gcs/GCSBackupRepository.java | 14 +++++--------- .../apache/solr/gcs/GCSIncrementalBackupTest.java | 8 +------- .../org/apache/solr/gcs/GCSInstallShardTest.java | 2 +- .../solr/gcs/LocalStorageGCSBackupRepository.java | 4 ++-- 4 files changed, 9 insertions(+), 19 deletions(-) diff --git a/solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java b/solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java index ae767aa2e432..18a5f2ce904e 100644 --- a/solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java +++ b/solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java @@ -182,9 +182,8 @@ public boolean exists(String path) throws IOException { if (path.endsWith("/")) { return storage.get(bucketName, path, Storage.BlobGetOption.fields()) != null; } else { - final String filePath = path; final String directoryPath = path + "/"; - return storage.get(bucketName, filePath, Storage.BlobGetOption.fields()) != null + return storage.get(bucketName, path, Storage.BlobGetOption.fields()) != null || storage.get(bucketName, directoryPath, Storage.BlobGetOption.fields()) != null; } } @@ -201,9 +200,7 @@ public PathType getPathType(URI path) throws IOException { @Override public String[] listAll(URI path) throws IOException { - final String blobName = appendTrailingSeparatorIfNecessary(path.toString()); - - final String pathStr = blobName; + final String pathStr = appendTrailingSeparatorIfNecessary(path.toString()); final List result = new ArrayList<>(); storage .list( @@ -231,10 +228,10 @@ public String[] listAll(URI path) throws IOException { @Override public IndexInput openInput(URI dirPath, String fileName, IOContext ctx) throws IOException { - return openInput(dirPath, fileName, ctx, readBufferSizeBytes); + return openInput(dirPath, fileName, readBufferSizeBytes); } - private IndexInput openInput(URI dirPath, String fileName, IOContext ctx, int bufferSize) { + private IndexInput openInput(URI dirPath, String fileName, int bufferSize) { String blobName = resolve(dirPath, fileName).toString(); final BlobId blobId = BlobId.of(bucketName, blobName); @@ -309,10 +306,9 @@ public void deleteDirectory(URI path) throws IOException { } protected List allBlobsAtDir(URI path) throws IOException { - final String blobName = appendTrailingSeparatorIfNecessary(path.toString()); + final String pathStr = appendTrailingSeparatorIfNecessary(path.toString()); final List result = new ArrayList<>(); - final String pathStr = blobName; storage .list(bucketName, Storage.BlobListOption.prefix(pathStr), Storage.BlobListOption.fields()) .iterateAll() diff --git a/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSIncrementalBackupTest.java b/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSIncrementalBackupTest.java index d08a019ba1cd..20f04dfd693a 100644 --- a/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSIncrementalBackupTest.java +++ b/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSIncrementalBackupTest.java @@ -18,13 +18,10 @@ package org.apache.solr.gcs; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakLingering; -import java.lang.invoke.MethodHandles; import org.apache.lucene.tests.util.LuceneTestCase; import org.apache.solr.cloud.api.collections.AbstractIncrementalBackupTest; import org.junit.AfterClass; import org.junit.BeforeClass; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; @LuceneTestCase.Nightly @ThreadLeakLingering(linger = 10) @@ -32,7 +29,6 @@ "SimpleText" }) // Backups do checksum validation against a footer value not present in 'SimpleText' public class GCSIncrementalBackupTest extends AbstractIncrementalBackupTest { - private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); public static final String SOLR_XML = "\n" + "\n" @@ -73,8 +69,6 @@ public class GCSIncrementalBackupTest extends AbstractIncrementalBackupTest { + " \n" + "\n"; - private static String backupLocation; - @BeforeClass public static void setupClass() throws Exception { // Enable parallel backup/restore for cloud storage tests @@ -88,7 +82,7 @@ public static void setupClass() throws Exception { } @AfterClass - public static void tearDownClass() throws Exception { + public static void tearDownClass() { LocalStorageGCSBackupRepository.clearStashedStorage(); } diff --git a/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSInstallShardTest.java b/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSInstallShardTest.java index 4b78c0cc8053..a797a2899254 100644 --- a/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSInstallShardTest.java +++ b/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSInstallShardTest.java @@ -67,7 +67,7 @@ public static void setupClass() throws Exception { } @AfterClass - public static void tearDownClass() throws Exception { + public static void tearDownClass() { LocalStorageGCSBackupRepository.clearStashedStorage(); } } diff --git a/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/LocalStorageGCSBackupRepository.java b/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/LocalStorageGCSBackupRepository.java index fc26a188f877..6c342653f915 100644 --- a/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/LocalStorageGCSBackupRepository.java +++ b/solr/modules/gcs-repository/src/test/org/apache/solr/gcs/LocalStorageGCSBackupRepository.java @@ -81,8 +81,8 @@ protected Storage getSingletonStorage() { } // FakeStorageRpc isn't thread-safe, which causes flaky test failures when multiple cores - // attempt to backup files - // simultaneously. We work around this here by wrapping it in a delegating instance that adds a + // attempt to back up files simultaneously. We work around this here by wrapping it in a + // delegating instance that adds a // measure of thread safety. stashedStorage = new ConcurrentDelegatingStorage(LocalStorageHelper.customOptions(false).getService());