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.

Sign up for GitHub

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

Jump to bottom

Snapshot : azure module - accelerate the listing of files (used in delete snapshot) #25710

Merged
merged 7 commits into from
Sep 11, 2017
Merged

Snapshot : azure module - accelerate the listing of files (used in delete snapshot) #25710

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
081f0e0
Update Integrations tests
etiennecarriere Jul 13, 2017
f676649
Update the ListBlob in Container in order to optimize the network…
etiennecarriere Jul 13, 2017
e6478a6
Corrections following the remarks on the pull request
etiennecarriere Jul 14, 2017
9e1a0a1
Remarks taken in account in the integration test
etiennecarriere Jul 14, 2017
3871628
Minor modifications on comments
etiennecarriere Aug 18, 2017
8e0ba87
Merge branch 'master' into 25424_azure_improve_delete_snapshot
etiennecarriere Aug 18, 2017
90e1290
Change to comply with the change of API
etiennecarriere Aug 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 6 additions & 14 deletions ...ry-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.microsoft.azure.storage.RetryExponentialRetry;
import com.microsoft.azure.storage.RetryPolicy;
import com.microsoft.azure.storage.StorageException;
import com.microsoft.azure.storage.blob.BlobListingDetails;
import com.microsoft.azure.storage.blob.BlobProperties;
import com.microsoft.azure.storage.blob.CloudBlobClient;
import com.microsoft.azure.storage.blob.CloudBlobContainer;
Expand All @@ -45,9 +46,7 @@
import java.io.OutputStream;
import java.net.URI;
import java.net.URISyntaxException;
import java.security.AccessController;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;

Expand Down Expand Up @@ -280,33 +279,26 @@ public Map<String, BlobMetaData> listBlobsByPrefix(String account, LocationMode

logger.debug("listing container [{}], keyPath [{}], prefix [{}]", container, keyPath, prefix);
MapBuilder<String, BlobMetaData> blobsBuilder = MapBuilder.newMapBuilder();
EnumSet<BlobListingDetails> enumBlobListingDetails = EnumSet.of(BlobListingDetails.METADATA);
CloudBlobClient client = this.getSelectedClient(account, mode);
CloudBlobContainer blobContainer = client.getContainerReference(container);

SocketAccess.doPrivilegedVoidException(() -> {
Copy link
Member

Choose a reason for hiding this comment

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

This doPrivileged needs to stay.

if (blobContainer.exists()) {
for (ListBlobItem blobItem : blobContainer.listBlobs(keyPath + (prefix == null ? "" : prefix))) {
for (ListBlobItem blobItem : blobContainer.listBlobs(keyPath + (prefix==null ? "" : prefix),false,
Copy link
Member

Choose a reason for hiding this comment

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

nit: please keep the spacing around == and between arguments (after ,)

enumBlobListingDetails, null, null)) {
URI uri = blobItem.getUri();
logger.trace("blob url [{}]", uri);

// uri.getPath is of the form /container/keyPath.* and we want to strip off the /container/
// this requires 1 + container.length() + 1, with each 1 corresponding to one of the /
String blobPath = uri.getPath().substring(1 + container.length() + 1);

CloudBlockBlob blob = blobContainer.getBlockBlobReference(blobPath);

// fetch the blob attributes from Azure (getBlockBlobReference does not do this)
// this is needed to retrieve the blob length (among other metadata) from Azure Storage
blob.downloadAttributes();

BlobProperties properties = blob.getProperties();
BlobProperties properties = ((CloudBlockBlob) blobItem).getProperties();
String name = blobPath.substring(keyPath.length());
logger.trace("blob url [{}], name [{}], size [{}]", uri, name, properties.getLength());
blobsBuilder.put(name, new PlainBlobMetaData(name, properties.getLength()));
}
}
});

return blobsBuilder.immutableMap();
}

Expand Down
74 changes: 48 additions & 26 deletions ...y-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,24 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeUnit;
import org.elasticsearch.plugin.repository.azure.AzureRepositoryPlugin;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.repositories.RepositoryMissingException;
import org.elasticsearch.repositories.RepositoryVerificationException;
import org.elasticsearch.repositories.azure.AzureRepository.Repository;
import org.elasticsearch.snapshots.SnapshotMissingException;
import org.elasticsearch.snapshots.SnapshotRestoreException;
import org.elasticsearch.snapshots.SnapshotState;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
import org.elasticsearch.test.store.MockFSDirectoryService;
import org.elasticsearch.test.store.MockFSIndexStore;
import org.junit.After;
import org.junit.Before;

import java.net.URISyntaxException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Locale;
import java.util.concurrent.TimeUnit;

Expand All @@ -64,13 +70,24 @@
supportsDedicatedMasters = false, numDataNodes = 1,
transportClientRatio = 0.0)
public class AzureSnapshotRestoreTests extends AbstractAzureWithThirdPartyIntegTestCase {

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return Arrays.asList(AzureRepositoryPlugin.class, MockFSIndexStore.TestPlugin.class);
}

private String getRepositoryPath() {
String testName = "it-" + getTestName();
return testName.contains(" ") ? Strings.split(testName, " ")[0] : testName;
}

public static String getContainerName() {
String testName = "snapshot-itest-".concat(RandomizedTest.getContext().getRunnerSeedAsString().toLowerCase(Locale.ROOT));
/* Have a different name per test so that there is no possible race condition. As the long can be negative,
* there mustn't be an hyphen between the 2 concat numbers
Copy link
Member

Choose a reason for hiding this comment

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

typo: an -> a, concat -> concatenated

* (can't have 2 consecutives hypens on Azure containers)
Copy link
Member

Choose a reason for hiding this comment

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

typo: hypens -> hyphens

*/
String testName = "snapshot-itest-"
.concat(RandomizedTest.getContext().getRunnerSeedAsString().toLowerCase(Locale.ROOT));
return testName.contains(" ") ? Strings.split(testName, " ")[0] : testName;
}

Expand All @@ -94,9 +111,10 @@ public final void wipeAzureRepositories() throws StorageException, URISyntaxExce
}

public void testSimpleWorkflow() {
String repo_name = "test-repo-simple";
Client client = client();
logger.info("--> creating azure repository with path [{}]", getRepositoryPath());
PutRepositoryResponse putRepositoryResponse = client.admin().cluster().preparePutRepository("test-repo")
PutRepositoryResponse putRepositoryResponse = client.admin().cluster().preparePutRepository(repo_name)
.setType("azure").setSettings(Settings.builder()
.put(Repository.CONTAINER_SETTING.getKey(), getContainerName())
.put(Repository.BASE_PATH_SETTING.getKey(), getRepositoryPath())
Expand All @@ -119,13 +137,13 @@ public void testSimpleWorkflow() {
assertThat(client.prepareSearch("test-idx-3").setSize(0).get().getHits().getTotalHits(), equalTo(100L));

logger.info("--> snapshot");
CreateSnapshotResponse createSnapshotResponse = client.admin().cluster().prepareCreateSnapshot("test-repo", "test-snap")
CreateSnapshotResponse createSnapshotResponse = client.admin().cluster().prepareCreateSnapshot(repo_name, "test-snap")
.setWaitForCompletion(true).setIndices("test-idx-*", "-test-idx-3").get();
assertThat(createSnapshotResponse.getSnapshotInfo().successfulShards(), greaterThan(0));
assertThat(createSnapshotResponse.getSnapshotInfo().successfulShards(),
equalTo(createSnapshotResponse.getSnapshotInfo().totalShards()));

assertThat(client.admin().cluster().prepareGetSnapshots("test-repo").setSnapshots("test-snap").get().getSnapshots()
assertThat(client.admin().cluster().prepareGetSnapshots(repo_name).setSnapshots("test-snap").get().getSnapshots()
.get(0).state(), equalTo(SnapshotState.SUCCESS));

logger.info("--> delete some data");
Expand All @@ -147,7 +165,7 @@ public void testSimpleWorkflow() {
client.admin().indices().prepareClose("test-idx-1", "test-idx-2").get();

logger.info("--> restore all indices from the snapshot");
RestoreSnapshotResponse restoreSnapshotResponse = client.admin().cluster().prepareRestoreSnapshot("test-repo", "test-snap")
RestoreSnapshotResponse restoreSnapshotResponse = client.admin().cluster().prepareRestoreSnapshot(repo_name, "test-snap")
.setWaitForCompletion(true).get();
assertThat(restoreSnapshotResponse.getRestoreInfo().totalShards(), greaterThan(0));

Expand All @@ -160,7 +178,7 @@ public void testSimpleWorkflow() {
logger.info("--> delete indices");
cluster().wipeIndices("test-idx-1", "test-idx-2");
logger.info("--> restore one index after deletion");
restoreSnapshotResponse = client.admin().cluster().prepareRestoreSnapshot("test-repo", "test-snap").setWaitForCompletion(true)
restoreSnapshotResponse = client.admin().cluster().prepareRestoreSnapshot(repo_name, "test-snap").setWaitForCompletion(true)
.setIndices("test-idx-*", "-test-idx-2").get();
assertThat(restoreSnapshotResponse.getRestoreInfo().totalShards(), greaterThan(0));
ensureGreen();
Expand All @@ -176,7 +194,7 @@ public void testSimpleWorkflow() {
public void testMultipleSnapshots() throws URISyntaxException, StorageException {
final String indexName = "test-idx-1";
final String typeName = "doc";
final String repositoryName = "test-repo";
final String repositoryName = "test-repo-multiple-snapshot";
final String snapshot1Name = "test-snap-1";
final String snapshot2Name = "test-snap-2";

Expand Down Expand Up @@ -313,6 +331,7 @@ public void testMultipleRepositories() {
* For issue #26: https://github.com/elastic/elasticsearch-cloud-azure/issues/26
*/
public void testListBlobs_26() throws StorageException, URISyntaxException {
final String repositoryName="test-repo-26";
createIndex("test-idx-1", "test-idx-2", "test-idx-3");
ensureGreen();

Expand All @@ -326,45 +345,45 @@ public void testListBlobs_26() throws StorageException, URISyntaxException {

ClusterAdminClient client = client().admin().cluster();
logger.info("--> creating azure repository without any path");
PutRepositoryResponse putRepositoryResponse = client.preparePutRepository("test-repo").setType("azure")
PutRepositoryResponse putRepositoryResponse = client.preparePutRepository(repositoryName).setType("azure")
.setSettings(Settings.builder()
.put(Repository.CONTAINER_SETTING.getKey(), getContainerName())
).get();
assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true));

// Get all snapshots - should be empty
assertThat(client.prepareGetSnapshots("test-repo").get().getSnapshots().size(), equalTo(0));
assertThat(client.prepareGetSnapshots(repositoryName).get().getSnapshots().size(), equalTo(0));

logger.info("--> snapshot");
CreateSnapshotResponse createSnapshotResponse = client.prepareCreateSnapshot("test-repo", "test-snap-26")
CreateSnapshotResponse createSnapshotResponse = client.prepareCreateSnapshot(repositoryName, "test-snap-26")
.setWaitForCompletion(true).setIndices("test-idx-*").get();
assertThat(createSnapshotResponse.getSnapshotInfo().successfulShards(), greaterThan(0));

// Get all snapshots - should have one
assertThat(client.prepareGetSnapshots("test-repo").get().getSnapshots().size(), equalTo(1));
assertThat(client.prepareGetSnapshots(repositoryName).get().getSnapshots().size(), equalTo(1));

// Clean the snapshot
client.prepareDeleteSnapshot("test-repo", "test-snap-26").get();
client.prepareDeleteRepository("test-repo").get();
client.prepareDeleteSnapshot(repositoryName, "test-snap-26").get();
client.prepareDeleteRepository(repositoryName).get();

logger.info("--> creating azure repository path [{}]", getRepositoryPath());
putRepositoryResponse = client.preparePutRepository("test-repo").setType("azure")
putRepositoryResponse = client.preparePutRepository(repositoryName).setType("azure")
.setSettings(Settings.builder()
.put(Repository.CONTAINER_SETTING.getKey(), getContainerName())
.put(Repository.BASE_PATH_SETTING.getKey(), getRepositoryPath())
).get();
assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true));

// Get all snapshots - should be empty
assertThat(client.prepareGetSnapshots("test-repo").get().getSnapshots().size(), equalTo(0));
assertThat(client.prepareGetSnapshots(repositoryName).get().getSnapshots().size(), equalTo(0));

logger.info("--> snapshot");
createSnapshotResponse = client.prepareCreateSnapshot("test-repo", "test-snap-26").setWaitForCompletion(true)
createSnapshotResponse = client.prepareCreateSnapshot(repositoryName, "test-snap-26").setWaitForCompletion(true)
.setIndices("test-idx-*").get();
assertThat(createSnapshotResponse.getSnapshotInfo().successfulShards(), greaterThan(0));

// Get all snapshots - should have one
assertThat(client.prepareGetSnapshots("test-repo").get().getSnapshots().size(), equalTo(1));
assertThat(client.prepareGetSnapshots(repositoryName).get().getSnapshots().size(), equalTo(1));


}
Expand All @@ -373,23 +392,24 @@ public void testListBlobs_26() throws StorageException, URISyntaxException {
* For issue #28: https://github.com/elastic/elasticsearch-cloud-azure/issues/28
*/
public void testGetDeleteNonExistingSnapshot_28() throws StorageException, URISyntaxException {
final String repositoryName="test-repo-28";
ClusterAdminClient client = client().admin().cluster();
logger.info("--> creating azure repository without any path");
PutRepositoryResponse putRepositoryResponse = client.preparePutRepository("test-repo").setType("azure")
PutRepositoryResponse putRepositoryResponse = client.preparePutRepository(repositoryName).setType("azure")
.setSettings(Settings.builder()
.put(Repository.CONTAINER_SETTING.getKey(), getContainerName())
).get();
assertThat(putRepositoryResponse.isAcknowledged(), equalTo(true));

try {
client.prepareGetSnapshots("test-repo").addSnapshots("nonexistingsnapshotname").get();
client.prepareGetSnapshots(repositoryName).addSnapshots("nonexistingsnapshotname").get();
fail("Shouldn't be here");
} catch (SnapshotMissingException ex) {
// Expected
}

try {
client.prepareDeleteSnapshot("test-repo", "nonexistingsnapshotname").get();
client.prepareDeleteSnapshot(repositoryName, "nonexistingsnapshotname").get();
fail("Shouldn't be here");
} catch (SnapshotMissingException ex) {
// Expected
Expand Down Expand Up @@ -418,18 +438,19 @@ public void testForbiddenContainerName() throws Exception {
* @param correct Is this container name correct
*/
private void checkContainerName(final String container, final boolean correct) throws Exception {
String repositoryName = "test-repo-checkContainerName";
logger.info("--> creating azure repository with container name [{}]", container);
// It could happen that we just removed from a previous test the same container so
// we can not create it yet.
assertBusy(() -> {
try {
PutRepositoryResponse putRepositoryResponse = client().admin().cluster().preparePutRepository("test-repo")
PutRepositoryResponse putRepositoryResponse = client().admin().cluster().preparePutRepository(repositoryName)
.setType("azure").setSettings(Settings.builder()
.put(Repository.CONTAINER_SETTING.getKey(), container)
.put(Repository.BASE_PATH_SETTING.getKey(), getRepositoryPath())
.put(Repository.CHUNK_SIZE_SETTING.getKey(), randomIntBetween(1000, 10000), ByteSizeUnit.BYTES)
).get();
client().admin().cluster().prepareDeleteRepository("test-repo").get();
client().admin().cluster().prepareDeleteRepository(repositoryName).get();
try {
logger.info("--> remove container [{}]", container);
cleanRepositoryFiles(container);
Expand All @@ -450,9 +471,10 @@ private void checkContainerName(final String container, final boolean correct) t
* Test case for issue #23: https://github.com/elastic/elasticsearch-cloud-azure/issues/23
*/
public void testNonExistingRepo_23() {
final String repositoryName = "test-repo-test23";
Client client = client();
logger.info("--> creating azure repository with path [{}]", getRepositoryPath());
PutRepositoryResponse putRepositoryResponse = client.admin().cluster().preparePutRepository("test-repo")
PutRepositoryResponse putRepositoryResponse = client.admin().cluster().preparePutRepository(repositoryName)
.setType("azure").setSettings(Settings.builder()
.put(Repository.CONTAINER_SETTING.getKey(), getContainerName())
.put(Repository.BASE_PATH_SETTING.getKey(), getRepositoryPath())
Expand All @@ -462,9 +484,9 @@ public void testNonExistingRepo_23() {

logger.info("--> restore non existing snapshot");
try {
client.admin().cluster().prepareRestoreSnapshot("test-repo", "no-existing-snapshot").setWaitForCompletion(true).get();
client.admin().cluster().prepareRestoreSnapshot(repositoryName, "no-existing-snapshot").setWaitForCompletion(true).get();
fail("Shouldn't be here");
} catch (SnapshotMissingException ex) {
} catch (SnapshotRestoreException ex) {
// Expected
}
}
Expand All @@ -474,7 +496,7 @@ public void testNonExistingRepo_23() {
*/
public void testRemoveAndCreateContainer() throws Exception {
final String container = getContainerName().concat("-testremove");
final AzureStorageService storageService = new AzureStorageServiceImpl(internalCluster().getDefaultSettings());
final AzureStorageService storageService = new AzureStorageServiceImpl(nodeSettings(0));

// It could happen that we run this test really close to a previous one
// so we might need some time to be able to create the container
Expand Down