Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Azure Bulk Deletes in Azure Repository #53919

Merged

Conversation

original-brownbear
Copy link
Member

Now that we upgraded the Azure SDK in #53865 we can start using bulk deletes like we do with S3 and GCS.

Upgrading to 8.6.2 in elastic#53865 broke running against HTTPs endpoints (and hence real azure)
because the https url connection needs the newly added permission to work.
Now that we upgraded the Azure SDK to 8.6.2 we can make use of
bulk deletes.
@original-brownbear original-brownbear added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.7.0 labels Mar 21, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

@@ -80,15 +74,6 @@ AzureStorageService createAzureStoreService(final Settings settings) {
);
}

@Override
public List<ExecutorBuilder<?>> getExecutorBuilders(Settings settings) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to deprecate here. We have never documented this threadpool and it was always seen as an internal setting.
Having it set will not prevent ES from starting up cleanly so I don't see anyone getting hurt here.

@@ -67,18 +72,21 @@ public void handle(final HttpExchange exchange) throws IOException {
assert read == -1 : "Request body should have been empty but saw [" + read + "]";
}
try {
// Request body is closed in the finally block
final BytesReference requestBody = Streams.readFully(Streams.noCloseStream(exchange.getRequestBody()));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we needed the request body again I refactored things like we did for the other mocks to full drain the input first thing here as well to keep things simple.

@@ -20,4 +20,5 @@
grant {
// azure client opens socket connections for to access repository
permission java.net.SocketPermission "*", "connect";
permission java.lang.RuntimePermission "setFactory";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #53903 this will go away once #53903 is merged, I just set it to be able to run 3rd party tests locally and verify this actually works now.

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm happy to see this gone. I trust your code on the server side logic of the batch request :) I left only minor comments.

++currentBatchSize;
} while (blobIterator.hasNext() && currentBatchSize < Constants.BATCH_MAX_REQUESTS);
currentBatchSize = 0;
logger.trace(() -> new ParameterizedMessage("delete blobs for container [{}], blob [{}]", container, blobs));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of more than 256 blobs to delete, this will log the same message multiple times right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right :) moved the log statement to the beginning of the method now

@@ -64,7 +64,7 @@ protected Settings repositorySettings() {

@Override
protected Map<String, HttpHandler> createHttpHandlers() {
return Collections.singletonMap("/container", new AzureBlobStoreHttpHandler("container"));
return Collections.singletonMap("/", new AzureBlobStoreHttpHandler("container"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Batch request does not prefix with the container name, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jup exactly, that's why I had to expand the context here

"Content-ID: " + contentId++ + " \r\n");
if (blobs.remove(b) == null) {
writer.write("\r\nHTTP/1.1 404 The specified blob does not exist. \r\n" +
"x-ms-error-code: BlobNotFound \r\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@original-brownbear
Copy link
Member Author

Thanks Yannick + Tanguy!

@original-brownbear original-brownbear merged commit 23cccf0 into elastic:master Mar 23, 2020
@original-brownbear original-brownbear deleted the azure-bulk-deletes-2 branch March 23, 2020 11:22
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Mar 23, 2020
Now that we upgraded the Azure SDK to 8.6.2 in elastic#53865 we can make use of
bulk deletes.
original-brownbear added a commit that referenced this pull request Mar 23, 2020
Now that we upgraded the Azure SDK to 8.6.2 in #53865 we can make use of
bulk deletes.
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Mar 24, 2020
This reverts commit 23cccf0.
Unfortunately SAS token auth still doesn't work with bulk deletes so we can't use them yet.

Closes elastic#54080
original-brownbear added a commit that referenced this pull request Mar 24, 2020
This reverts commit 23cccf0.
Unfortunately SAS token auth still doesn't work with bulk deletes so we can't use them yet.

Closes #54080
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Mar 24, 2020
…lastic#54089)

This reverts commit 23cccf0.
Unfortunately SAS token auth still doesn't work with bulk deletes so we can't use them yet.

Closes elastic#54080
original-brownbear added a commit that referenced this pull request Mar 25, 2020
…#54111)

This reverts commit 23cccf0.
Unfortunately SAS token auth still doesn't work with bulk deletes so we can't use them yet.

Closes #54080
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Mar 26, 2020
…lastic#54089)

This reverts commit 23cccf0.
Unfortunately SAS token auth still doesn't work with bulk deletes so we can't use them yet.

Closes elastic#54080
original-brownbear added a commit that referenced this pull request Mar 26, 2020
…#54248)

This reverts commit 23cccf0.
Unfortunately SAS token auth still doesn't work with bulk deletes so we can't use them yet.

Closes #54080
yyff pushed a commit to yyff/elasticsearch that referenced this pull request Apr 17, 2020
Now that we upgraded the Azure SDK to 8.6.2 in elastic#53865 we can make use of
bulk deletes.
yyff pushed a commit to yyff/elasticsearch that referenced this pull request Apr 17, 2020
…lastic#54089)

This reverts commit 23cccf0.
Unfortunately SAS token auth still doesn't work with bulk deletes so we can't use them yet.

Closes elastic#54080
@original-brownbear original-brownbear restored the azure-bulk-deletes-2 branch August 6, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants