Skip to content

Commit 53037d6

Browse files
SOLR-17949: Fix Azure Blob path/listing bugs and wire tests to shared backup suites
Align the Azure Blob backup repository with S3/GCS conventions: - resolve()/getBlobPath() now mirror S3 exactly (fold URI host into the path), fixing exists()/getPathType() on virtual directories - listDir() strips trailing delimiters so it returns bare child names per the BackupRepository contract - delete() tolerates already-absent files (lenient, like local/S3) - commitBlockList(..., true) for overwrite semantics on retried uploads - handleBlobException() logs HTTP 404 at debug, other failures at error - fail fast with a clear message when the container name is missing - drop the unused CHUNK_SIZE constant and unused throws on sanitizedPath() - document the ~195 GiB single-file block-count ceiling Rewrite the tests to extend the shared abstract suites (AbstractBackupRepositoryTest, AbstractIncrementalBackupTest, AbstractInstallShardTest) against Azurite via a new AzuriteTestContainer helper. Recommend supplying secrets via sysprops/env vars in the reference guide. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 0ee9d20 commit 53037d6

13 files changed

Lines changed: 497 additions & 968 deletions

solr/modules/azure-blob-repository/build.gradle

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ dependencies {
6565
testImplementation libs.commonsio.commonsio
6666

6767
testImplementation libs.azure.core.http.okhttp
68-
testImplementation libs.squareup.okhttp3.okhttp
68+
testImplementation libs.squareup.okhttp3.okhttp.jvm
6969

7070
// Testcontainers for Azurite integration testing
7171
testImplementation libs.testcontainers

solr/modules/azure-blob-repository/gradle.lockfile

Lines changed: 73 additions & 46 deletions
Large diffs are not rendered by default.

solr/modules/azure-blob-repository/src/java/org/apache/solr/azureblob/AzureBlobBackupRepository.java

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ public class AzureBlobBackupRepository extends AbstractBackupRepository {
5353
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
5454

5555
static final String BLOB_SCHEME = "blob";
56-
private static final int CHUNK_SIZE = 16 * 1024 * 1024;
5756
private static final int COPY_BUFFER_SIZE = 8192;
5857

5958
private AzureBlobStorageClient client;
@@ -121,6 +120,7 @@ public URI resolve(URI baseUri, String... pathComponents) {
121120
throw new IllegalArgumentException("URI must begin with 'blob:' scheme");
122121
}
123122

123+
// If paths contain unnecessary '/' separators, they'll be removed by URI.normalize()
124124
String path = baseUri + "/" + String.join("/", pathComponents);
125125
return URI.create(path).normalize();
126126
}
@@ -170,30 +170,26 @@ public void delete(URI path, Collection<String> files) throws IOException {
170170
Objects.requireNonNull(path, "cannot delete with a null URI");
171171
Objects.requireNonNull(files, "cannot delete with a null files collection");
172172

173-
String basePath = getBlobPath(path);
174-
175-
if (!client.isDirectory(basePath)) {
176-
int lastSlash = basePath.lastIndexOf('/');
177-
basePath = lastSlash >= 0 ? basePath.substring(0, lastSlash) : "";
178-
}
179-
180-
final String prefix;
181-
if (basePath.isEmpty() || basePath.endsWith("/")) {
182-
prefix = basePath;
183-
} else {
184-
prefix = basePath + "/";
185-
}
186-
187173
Set<String> fullPaths =
188174
files.stream()
189-
.map(file -> prefix + file.replaceFirst("^/+", ""))
175+
.map(file -> resolve(path, file))
176+
.map(this::getBlobPath)
190177
.collect(Collectors.toSet());
191178

192179
if (log.isDebugEnabled()) {
193180
log.debug("Delete files '{}'", fullPaths);
194181
}
195182

196-
client.delete(fullPaths);
183+
try {
184+
client.delete(fullPaths);
185+
} catch (AzureBlobNotFoundException e) {
186+
// Deleting files that are already absent is a no-op at the repository level, matching the
187+
// lenient behavior of the local-filesystem and S3 repositories. Any present files in the
188+
// batch were still removed before this was thrown.
189+
if (log.isDebugEnabled()) {
190+
log.debug("Some files requested for deletion were already absent", e);
191+
}
192+
}
197193
}
198194

199195
@Override
@@ -352,7 +348,7 @@ public void copyIndexFileTo(
352348

353349
try (InputStream inputStream = client.pullStream(blobPath);
354350
IndexOutput indexOutput = dest.createOutput(destFileName, IOContext.DEFAULT)) {
355-
byte[] buffer = new byte[CHUNK_SIZE];
351+
byte[] buffer = new byte[COPY_BUFFER_SIZE];
356352
int len;
357353
while ((len = inputStream.read(buffer)) != -1) {
358354
indexOutput.writeBytes(buffer, 0, len);
@@ -377,7 +373,10 @@ private String getBlobPath(URI uri) {
377373
if (!BLOB_SCHEME.equalsIgnoreCase(uri.getScheme())) {
378374
throw new IllegalArgumentException("URI must begin with 'blob:' scheme");
379375
}
380-
return uri.getPath();
376+
// Depending on the scheme, the first path element may be parsed as the URI host (e.g.
377+
// "blob://dir/file" -> host="dir"). Fold it back into the path, mirroring S3BackupRepository.
378+
String host = uri.getHost();
379+
return host == null ? uri.getPath() : host + uri.getPath();
381380
}
382381

383382
private void writeFooter(long checksum, OutputStream outputStream) throws IOException {

solr/modules/azure-blob-repository/src/java/org/apache/solr/azureblob/AzureBlobBackupRepositoryConfig.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import org.apache.solr.common.util.EnvUtils;
2020
import org.apache.solr.common.util.NamedList;
21+
import org.apache.solr.common.util.StrUtils;
2122

2223
public class AzureBlobBackupRepositoryConfig {
2324

@@ -54,6 +55,12 @@ public AzureBlobBackupRepositoryConfig(NamedList<?> config) {
5455
}
5556

5657
public AzureBlobStorageClient buildClient() {
58+
if (StrUtils.isNullOrEmpty(containerName)) {
59+
throw new IllegalArgumentException(
60+
"Missing required configuration '"
61+
+ CONTAINER_NAME
62+
+ "' for the Azure Blob backup repository");
63+
}
5764
return new AzureBlobStorageClient(
5865
containerName,
5966
connectionString,

solr/modules/azure-blob-repository/src/java/org/apache/solr/azureblob/AzureBlobOutputStream.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,7 @@
3838
*/
3939
class AzureBlobOutputStream extends OutputStream {
4040
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
41-
42-
static final int BLOCK_SIZE = 4 * 1024 * 1024;
41+
private static final int BLOCK_SIZE = 4 * 1024 * 1024;
4342

4443
private final BlobClient blobClient;
4544
private final String blobPath;
@@ -220,7 +219,7 @@ void complete() throws IOException {
220219

221220
try {
222221
BlockBlobClient blockBlobClient = blobClient.getBlockBlobClient();
223-
blockBlobClient.commitBlockList(blockIds);
222+
blockBlobClient.commitBlockList(blockIds, true);
224223
} catch (BlobStorageException e) {
225224
throw new IOException(
226225
"Failed to commit block list", AzureBlobStorageClient.handleBlobException(e));

solr/modules/azure-blob-repository/src/java/org/apache/solr/azureblob/AzureBlobStorageClient.java

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,8 @@ String[] listDir(String path) throws AzureBlobException {
222222
int slashIndex = s.indexOf(BLOB_FILE_PATH_DELIMITER);
223223
return slashIndex == -1 || slashIndex == s.length() - 1;
224224
})
225+
.map(s -> s.endsWith(BLOB_FILE_PATH_DELIMITER) ? s.substring(0, s.length() - 1) : s)
226+
.distinct()
225227
.toArray(String[]::new);
226228
} catch (BlobStorageException e) {
227229
throw handleBlobException(e);
@@ -579,7 +581,7 @@ private String getParentDirectory(String path) {
579581
: "";
580582
}
581583

582-
String sanitizedPath(String path) throws AzureBlobException {
584+
String sanitizedPath(String path) {
583585
String sanitizedPath = path.trim();
584586
while (sanitizedPath.startsWith(BLOB_FILE_PATH_DELIMITER)) {
585587
sanitizedPath = sanitizedPath.substring(1).trim();
@@ -621,12 +623,14 @@ static AzureBlobException handleBlobException(BlobStorageException e) {
621623
e.getErrorCode(),
622624
e.getMessage());
623625

624-
log.error(errMessage);
625-
626626
if (e.getStatusCode() == HTTP_NOT_FOUND) {
627+
if (log.isDebugEnabled()) {
628+
log.debug(errMessage);
629+
}
627630
return new AzureBlobNotFoundException(errMessage, e);
628-
} else {
629-
return new AzureBlobException(errMessage, e);
630631
}
632+
633+
log.error(errMessage);
634+
return new AzureBlobException(errMessage, e);
631635
}
632636
}

solr/modules/azure-blob-repository/src/test/org/apache/solr/azureblob/AbstractAzureBlobClientTest.java

Lines changed: 11 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,6 @@
3737
import org.junit.Assume;
3838
import org.junit.Before;
3939
import org.junit.BeforeClass;
40-
import org.testcontainers.containers.GenericContainer;
41-
import org.testcontainers.utility.DockerImageName;
4240

4341
/** Abstract class for tests with Azure Blob Storage emulator. */
4442
@ThreadLeakFilters(
@@ -50,10 +48,7 @@
5048
})
5149
public class AbstractAzureBlobClientTest extends SolrTestCase {
5250

53-
private static final String AZURITE_IMAGE = "mcr.microsoft.com/azure-storage/azurite:3.35.0";
54-
private static final int BLOB_SERVICE_PORT = 10000;
55-
56-
private static GenericContainer<?> azuriteContainer;
51+
private static AzuriteTestContainer azurite;
5752
private static OkHttpClient sharedOkHttpClient;
5853
private static String connectionString;
5954

@@ -62,15 +57,10 @@ public class AbstractAzureBlobClientTest extends SolrTestCase {
6257

6358
protected AzureBlobStorageClient client;
6459

65-
@SuppressWarnings("resource")
6660
@BeforeClass
6761
public static void setUpClass() {
6862
try {
69-
azuriteContainer =
70-
new GenericContainer<>(DockerImageName.parse(AZURITE_IMAGE))
71-
.withExposedPorts(BLOB_SERVICE_PORT)
72-
.withCommand("azurite-blob", "--blobHost", "0.0.0.0", "--skipApiVersionCheck");
73-
azuriteContainer.start();
63+
azurite = AzuriteTestContainer.start();
7464
sharedOkHttpClient = new OkHttpClient.Builder().build();
7565
} catch (Throwable t) {
7666
Assume.assumeNoException("Docker/Testcontainers not available; skipping Azure tests", t);
@@ -81,20 +71,17 @@ public static void setUpClass() {
8171
public void setUpClient() throws Exception {
8272
setAzureTestCredentials();
8373

84-
String blobServiceUrl = getBlobServiceUrl();
85-
connectionString =
86-
"DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;BlobEndpoint="
87-
+ blobServiceUrl
88-
+ "/devstoreaccount1;";
74+
URI blobServiceUri = new URI(getBlobServiceUrl());
75+
connectionString = azurite.connectionString();
8976

9077
proxy = new SocketProxy();
91-
proxy.open(new URI(blobServiceUrl));
78+
proxy.open(blobServiceUri);
9279

9380
HttpClient httpClient = new OkHttpAsyncHttpClientBuilder(sharedOkHttpClient).build();
9481

82+
// Route the client through the proxy so tests can simulate connection loss.
9583
String proxiedConn =
96-
connectionString.replace(
97-
":" + azuriteContainer.getMappedPort(BLOB_SERVICE_PORT), ":" + proxy.getListenPort());
84+
connectionString.replace(":" + blobServiceUri.getPort(), ":" + proxy.getListenPort());
9885

9986
BlobServiceClient blobServiceClient =
10087
new BlobServiceClientBuilder()
@@ -137,13 +124,9 @@ void initiateBlobConnectionLoss() {
137124

138125
@AfterClass
139126
public static void afterAll() {
140-
if (azuriteContainer != null) {
141-
try {
142-
azuriteContainer.stop();
143-
azuriteContainer.close();
144-
} catch (Throwable ignored) {
145-
}
146-
azuriteContainer = null;
127+
if (azurite != null) {
128+
azurite.stop();
129+
azurite = null;
147130
}
148131
sharedOkHttpClient = null;
149132
}
@@ -165,10 +148,7 @@ static String getConnectionString() {
165148
}
166149

167150
String getBlobServiceUrl() {
168-
return "http://"
169-
+ azuriteContainer.getHost()
170-
+ ":"
171-
+ azuriteContainer.getMappedPort(BLOB_SERVICE_PORT);
151+
return azurite.blobEndpoint();
172152
}
173153

174154
public static class OkHttpThreadLeakFilterTest implements ThreadFilter {

0 commit comments

Comments
 (0)