Skip to content

Commit a5549e4

Browse files
mhl-bmhl-b
committed
tests and feedback
1 parent 6f2134c commit a5549e4

File tree

3 files changed

+67
-50
lines changed

3 files changed

+67
-50
lines changed

‎modules/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStore.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -384,8 +384,6 @@ private void initResumableStream() throws IOException {
384384
public void write(byte[] b, int off, int len) throws IOException {
385385
int written = 0;
386386
while (written < len) {
387-
// at most write the default chunk size in one go to prevent allocating huge buffers in the SDK
388-
// see com.google.cloud.BaseWriteChannel#DEFAULT_CHUNK_SIZE
389387
final int toWrite = Math.min(len - written, SDK_DEFAULT_CHUNK_SIZE);
390388
out.write(b, off + written, toWrite);
391389
written += toWrite;

‎modules/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageHttpStatsCollector.java‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ final class GoogleCloudStorageHttpStatsCollector implements HttpResponseIntercep
2222
private final GoogleCloudStorageOperationsStats stats;
2323
private final OperationPurpose purpose;
2424
private final Pattern getObjPattern;
25-
private final Pattern insertObPattern;
25+
private final Pattern insertObjPattern;
2626
private final Pattern listObjPattern;
2727

2828
GoogleCloudStorageHttpStatsCollector(final GoogleCloudStorageOperationsStats stats, OperationPurpose purpose) {
@@ -33,7 +33,7 @@ final class GoogleCloudStorageHttpStatsCollector implements HttpResponseIntercep
3333
// The specification for the current API (v1) endpoints can be found at:
3434
// https://cloud.google.com/storage/docs/json_api/v1
3535
this.getObjPattern = Pattern.compile("(/download)?/storage/v1/b/" + bucket + "/o/.+");
36-
this.insertObPattern = Pattern.compile("(/upload)?/storage/v1/b/" + bucket + "/o");
36+
this.insertObjPattern = Pattern.compile("(/upload)?/storage/v1/b/" + bucket + "/o");
3737
this.listObjPattern = Pattern.compile("/storage/v1/b/" + bucket + "/o");
3838
}
3939

@@ -49,42 +49,44 @@ private void trackRequestAndOperation(Operation operation) {
4949
public void interceptResponse(final HttpResponse response) {
5050
var request = response.getRequest();
5151
var path = request.getUrl().getRawPath();
52+
var ignored = false;
5253
switch (request.getRequestMethod()) {
5354
case "GET" -> {
5455
// https://cloud.google.com/storage/docs/json_api/v1/objects/get
5556
if (getObjPattern.matcher(path).matches()) {
5657
// Retrieves object metadata. When alt=media is included as a query parameter, retrieves object data.
57-
if (request.getUrl().getFirst("alt").equals("media")) {
58+
if ("media".equals(request.getUrl().getFirst("alt"))) {
5859
trackRequestAndOperation(Operation.GET_OBJECT);
5960
} else {
6061
trackRequestAndOperation(Operation.GET_METADATA);
6162
}
6263
} else if (listObjPattern.matcher(path).matches()) {
6364
trackRequestAndOperation(Operation.LIST_OBJECTS);
65+
} else {
66+
ignored = true;
6467
}
65-
// ignore other get requests
6668
}
6769
case "POST", "PUT" -> {
6870
// https://cloud.google.com/storage/docs/json_api/v1/objects/insert
69-
if (insertObPattern.matcher(path).matches()) {
71+
if (insertObjPattern.matcher(path).matches()) {
7072
var obj = request.getUrl().getFirst("uploadType");
7173
if (obj instanceof String uploadType) {
7274
switch (uploadType) {
7375
// We dont track insert operations here, only requests. The reason is billing impact.
7476
// Any insert, including multipart or resumable parts, are counted as one operation.
7577
case "multipart" -> trackRequest(Operation.MULTIPART_UPLOAD);
7678
case "resumable" -> trackRequest(Operation.RESUMABLE_UPLOAD);
77-
default -> {
78-
// ignore "media" - Data-only upload. Upload the object data only, without any metadata.
79-
}
79+
default -> ignored = true;
8080
}
81+
} else {
82+
ignored = true;
8183
}
84+
} else {
85+
ignored = true;
8286
}
83-
// ignore other post requests
84-
}
85-
default -> {
86-
// ignore other http methods
8787
}
88+
default -> ignored = true;
8889
}
90+
assert ignored == false : "must handle response: " + request.getRequestMethod() + " " + path;
8991
}
9092
}

‎modules/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainerStatsTests.java‎

Lines changed: 53 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,9 @@
5353
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageClientSettings.PROJECT_ID_SETTING;
5454
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageClientSettings.READ_TIMEOUT_SETTING;
5555
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageOperationsStats.Operation;
56+
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageOperationsStats.Operation.GET_METADATA;
5657
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageOperationsStats.Operation.GET_OBJECT;
58+
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageOperationsStats.Operation.LIST_OBJECTS;
5759
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageOperationsStats.Operation.MULTIPART_UPLOAD;
5860
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageOperationsStats.Operation.RESUMABLE_UPLOAD;
5961

@@ -129,12 +131,12 @@ public void testSingleMultipartWrite() throws Exception {
129131
final BytesArray blobContents = new BytesArray(randomByteArrayOfLength(blobLength));
130132
container.writeBlob(purpose, blobName, blobContents, true);
131133

132-
var wantStats = new StatsMap();
133-
assertStatsEquals(wantStats.add(purpose, MULTIPART_UPLOAD, 1, 1), store.stats());
134+
final StatsMap wantStats = new StatsMap(purpose);
135+
assertStatsEquals(wantStats.add(MULTIPART_UPLOAD, 1, 1), store.stats());
134136
try (InputStream is = container.readBlob(purpose, blobName)) {
135137
assertEquals(blobContents, Streams.readFully(is));
136138
}
137-
assertStatsEquals(wantStats.add(purpose, GET_OBJECT, 1, 1), store.stats());
139+
assertStatsEquals(wantStats.add(GET_OBJECT, 1, 1), store.stats());
138140
}
139141

140142
@Test
@@ -153,14 +155,14 @@ public void testResumableWrite() throws Exception {
153155

154156
// a resumable upload sends at least 2 requests, a POST with metadata only and multiple PUTs with SDK_DEFAULT_CHUNK_SIZE
155157
// the +1 means a POST request with metadata without PAYLOAD
156-
var totalRequests = parts + 1;
157-
var wantStats = new StatsMap();
158-
assertStatsEquals(wantStats.add(purpose, RESUMABLE_UPLOAD, 1, totalRequests), store.stats());
158+
final int totalRequests = parts + 1;
159+
final StatsMap wantStats = new StatsMap(purpose);
160+
assertStatsEquals(wantStats.add(RESUMABLE_UPLOAD, 1, totalRequests), store.stats());
159161

160162
try (InputStream is = container.readBlob(purpose, blobName)) {
161163
assertEquals(blobContents, Streams.readFully(is));
162164
}
163-
assertStatsEquals(wantStats.add(purpose, GET_OBJECT, 1, 1), store.stats());
165+
assertStatsEquals(wantStats.add(GET_OBJECT, 1, 1), store.stats());
164166
}
165167

166168
@Test
@@ -171,14 +173,16 @@ public void testDeleteDirectory() throws Exception {
171173
final String directoryName = randomIdentifier();
172174
final BytesArray contents = new BytesArray(randomByteArrayOfLength(50));
173175
final int numberOfFiles = randomIntBetween(1, 20);
176+
final OperationPurpose purpose = randomPurpose();
174177
for (int i = 0; i < numberOfFiles; i++) {
175-
container.writeBlob(randomPurpose(), String.format("%s/file_%d", directoryName, i), contents, true);
178+
container.writeBlob(purpose, String.format("%s/file_%d", directoryName, i), contents, true);
176179
}
177-
assertEquals(createStats(numberOfFiles, 0, 0), store.stats());
180+
final StatsMap wantStats = new StatsMap(purpose);
181+
assertStatsEquals(wantStats.add(MULTIPART_UPLOAD, numberOfFiles, numberOfFiles), store.stats());
178182

179-
container.delete(randomPurpose());
183+
container.delete(purpose);
180184
// We only count the list because we can't track the bulk delete
181-
assertEquals(createStats(numberOfFiles, 1, 0), store.stats());
185+
assertStatsEquals(wantStats.add(LIST_OBJECTS, 1, 1), store.stats());
182186
}
183187

184188
@Test
@@ -191,15 +195,18 @@ public void testListBlobsAccountsForPaging() throws Exception {
191195
final int numberOfPages = randomIntBetween(1, 10);
192196
final int numberOfObjects = randomIntBetween((numberOfPages - 1) * pageSize, numberOfPages * pageSize - 1);
193197
final BytesArray contents = new BytesArray(randomByteArrayOfLength(50));
198+
final OperationPurpose purpose = randomPurpose();
194199
for (int i = 0; i < numberOfObjects; i++) {
195-
container.writeBlob(randomPurpose(), String.format("file_%d", i), contents, true);
200+
container.writeBlob(purpose, String.format("file_%d", i), contents, true);
196201
}
197-
assertEquals(createStats(numberOfObjects, 0, 0), store.stats());
202+
final StatsMap wantStats = new StatsMap(purpose);
203+
assertStatsEquals(wantStats.add(MULTIPART_UPLOAD, numberOfObjects, numberOfObjects), store.stats());
198204

199-
final Map<String, BlobMetadata> stringBlobMetadataMap = container.listBlobs(randomPurpose());
205+
final Map<String, BlobMetadata> stringBlobMetadataMap = container.listBlobs(purpose);
200206
assertEquals(numberOfObjects, stringBlobMetadataMap.size());
207+
201208
// There should be {numberOfPages} pages of blobs
202-
assertEquals(createStats(numberOfObjects, numberOfPages, 0), store.stats());
209+
assertStatsEquals(wantStats.add(LIST_OBJECTS, numberOfPages, numberOfPages), store.stats());
203210
}
204211

205212
public void testCompareAndSetRegister() {
@@ -209,32 +216,23 @@ public void testCompareAndSetRegister() {
209216
// update from empty (adds a single insert)
210217
final BytesArray contents = new BytesArray(randomByteArrayOfLength(BlobContainerUtils.MAX_REGISTER_CONTENT_LENGTH));
211218
final String registerName = randomIdentifier();
212-
assertTrue(safeAwait(l -> container.compareAndSetRegister(randomPurpose(), registerName, BytesArray.EMPTY, contents, l)));
213-
assertEquals(createStats(1, 0, 0), store.stats());
219+
final OperationPurpose purpose = randomPurpose();
220+
assertTrue(safeAwait(l -> container.compareAndSetRegister(purpose, registerName, BytesArray.EMPTY, contents, l)));
221+
final StatsMap wantStat = new StatsMap(purpose);
222+
assertStatsEquals(wantStat.add(GET_METADATA, 1, 1).add(MULTIPART_UPLOAD, 1, 1), store.stats());
214223

215224
// successful update from non-null (adds two gets, one insert)
216225
final BytesArray nextContents = new BytesArray(randomByteArrayOfLength(BlobContainerUtils.MAX_REGISTER_CONTENT_LENGTH));
217-
assertTrue(safeAwait(l -> container.compareAndSetRegister(randomPurpose(), registerName, contents, nextContents, l)));
218-
assertEquals(createStats(2, 0, 2), store.stats());
226+
assertTrue(safeAwait(l -> container.compareAndSetRegister(purpose, registerName, contents, nextContents, l)));
227+
assertStatsEquals(wantStat.add(GET_METADATA, 1, 1).add(GET_OBJECT, 1, 1).add(MULTIPART_UPLOAD, 1, 1), store.stats());
219228

220229
// failed update (adds two gets, zero inserts)
221230
final BytesArray wrongContents = randomValueOtherThan(
222231
nextContents,
223232
() -> new BytesArray(randomByteArrayOfLength(BlobContainerUtils.MAX_REGISTER_CONTENT_LENGTH))
224233
);
225-
assertFalse(safeAwait(l -> container.compareAndSetRegister(randomPurpose(), registerName, wrongContents, contents, l)));
226-
assertEquals(createStats(2, 0, 4), store.stats());
227-
}
228-
229-
private Map<String, BlobStoreActionStats> createStats(int insertCount, int listCount, int getCount) {
230-
return Map.of(
231-
"GetObject",
232-
new BlobStoreActionStats(getCount, getCount),
233-
"ListObjects",
234-
new BlobStoreActionStats(listCount, listCount),
235-
"InsertObject",
236-
new BlobStoreActionStats(insertCount, insertCount)
237-
);
234+
assertFalse(safeAwait(l -> container.compareAndSetRegister(purpose, registerName, wrongContents, contents, l)));
235+
assertStatsEquals(wantStat.add(GET_METADATA, 1, 1).add(GET_OBJECT, 1, 1), store.stats());
238236
}
239237

240238
private ContainerAndBlobStore createBlobContainer(final String repositoryName) throws Exception {
@@ -276,18 +274,37 @@ protected String getEndpointForServer(final HttpServer server) {
276274
}
277275

278276
static class StatsMap extends HashMap<String, BlobStoreActionStats> {
277+
private final OperationPurpose purpose;
278+
279279
StatsMap() {
280-
for (var purpose : OperationPurpose.values()) {
281-
for (var operation : Operation.values()) {
282-
put(purpose + "_" + operation, new BlobStoreActionStats(0, 0));
280+
this(null);
281+
}
282+
283+
StatsMap(OperationPurpose purpose) {
284+
this.purpose = purpose;
285+
for (var p : OperationPurpose.values()) {
286+
for (var o : Operation.values()) {
287+
put(p + "_" + o, new BlobStoreActionStats(0, 0));
283288
}
284289
}
285290
}
286291

287292
StatsMap add(OperationPurpose purpose, Operation operation, long ops, long reqs) {
288-
put(purpose + "_" + operation, new BlobStoreActionStats(ops, reqs));
293+
compute(purpose + "_" + operation, (k, v) -> {
294+
BlobStoreActionStats stats;
295+
if (v == null) {
296+
stats = new BlobStoreActionStats(ops, reqs);
297+
} else {
298+
stats = new BlobStoreActionStats(v.operations() + ops, v.requests() + reqs);
299+
}
300+
return stats;
301+
});
289302
return this;
290303
}
304+
305+
StatsMap add(Operation operation, long ops, long reqs) {
306+
return add(purpose, operation, ops, reqs);
307+
}
291308
}
292309

293310
private record ContainerAndBlobStore(GoogleCloudStorageBlobContainer blobContainer, GoogleCloudStorageBlobStore blobStore)

0 commit comments

Comments
 (0)