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

etiennecarriere · 2017-07-13T15:34:55Z

Current behaviour of the Listing of a container in azure plugin :

List all the blob
For each blob, request the medata of blob
=> it is ineffective as we have N+1 requests for N blobs

Proposed behaviour

List all blob and retrieving the metadata in the same request

…quests

elasticmachine · 2017-07-13T15:34:58Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

etiennecarriere · 2017-07-13T16:12:32Z

@karmi , My company signed a CLA ("A company wanting its employees to contribute") with my Github account in the annex ". Do I have to sign an other one (" An individual contributing under an existing company contributor agreement") ?

rjernst

Thanks for the PR! I left some comments. In general it looks like the idea is to call a different listBlobs impl which can return all the metadata of the keys along with the key itself. Can you please update the description to explain that?

rjernst · 2017-07-13T20:46:50Z

...itory-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java

        CloudBlobClient client = this.getSelectedClient(account, mode);
        CloudBlobContainer blobContainer = client.getContainerReference(container);
-
-        SocketAccess.doPrivilegedVoidException(() -> {


This doPrivileged needs to stay.

rjernst · 2017-07-13T20:53:09Z

...itory-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java

+                // 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);
+                if (!(blobItem instanceof CloudBlockBlob)){


I don't think we need this check, it should be an error if we don't get back CloudBlockBlob (which is what we expect for all keys here). I think it can be a hard assert.

I agree it is the assumption on the previous version of code but I prefer to be defensive : if the azure storage container has been modified by an outside component, it could have other type of blob (Append/Page) so I propose ignoring it and put a warning.

I am +1 on what @rjernst said - silently ignoring is not the way to go here, very few read log messages, its more important to fail hard if something changed that alters our underlying assumptions of what object type is returned there.

rjernst · 2017-07-13T20:55:10Z

...itory-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java

-                    logger.trace("blob url [{}], name [{}], size [{}]", uri, name, properties.getLength());
-                    blobsBuilder.put(name, new PlainBlobMetaData(name, properties.getLength()));
+        if (blobContainer.exists()) {
+            for (ListBlobItem blobItem : blobContainer.listBlobs(keyPath + (prefix == null ? "" : prefix),false,enumBlobListingDetails,null,null)) {


nit: please use spaces after the commas

Also, check the length of this line, it looks close to the 140 char limit

rjernst · 2017-07-13T20:59:06Z

...tory-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java

+         */
+        String testName = "snapshot-itest-"
+            .concat(RandomizedTest.getContext().getRunnerSeedAsString().toLowerCase(Locale.ROOT))
+            .concat(new Long(RandomizedTest.getContext().getRandom().nextLong()).toString());


This can simply be randomLong(). However, I don't think this will work because the cleanup in wipeAzureRepositories() would then not know about the generated name. If these need to be unique, then this method needs to stash the generated name so that the repository can be cleaned up after the test.

you are right : I rollback as it will not work in wipeAzureRepository .

rjernst · 2017-07-13T20:59:35Z

...tory-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java

        return testName.contains(" ") ? Strings.split(testName, " ")[0] : testName;
    }

-    @Override
-    public Settings indexSettings() {


I don't think these should be removed, but if they should, then please explain why.

After reflexion, ok to keep it but we have to add the MockFSIndexStore.TestPlugin so that the configuration variable are known. If we don't have, we have random error as MockFSIndexStore.TestPlugin is added randomly in ESIntegTestCase superclass

rjernst · 2017-07-13T21:03:02Z

@etiennecarriere Regarding the CLA, I believe you need to associate your github profile with the company signed CLA. If you follow the link to the CLA, I think that is the second option An individual contributing under an existing company contributor agreement.

karmi · 2017-07-14T02:19:04Z

Hi @etiennecarriere, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

etiennecarriere · 2017-07-14T02:30:58Z

@rjernst, @abeyad , I applied your comments for the code on the "main" class so the gradle check / test works.
On the tests, I had to modify 2 weeks ago in order to have functional integration tests. Unfortunately, I don't achieve to reproduce them today even on master without my modifications.

etiennecarriere · 2017-07-14T02:31:38Z

@karmi, my fault, I commited the corrections with my personal account and not my professional account.

karmi · 2017-07-17T16:25:47Z

@Etienne-Carriere, the check is now green, thanks for fixing the e-mail problem!

rjernst

@Etienne-Carriere Thanks for the updates and sorry for the delay in reviewing again. I left a couple more minor comments. If you can address them, I will merge this.

rjernst · 2017-08-17T23:06:27Z

...itory-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java

        SocketAccess.doPrivilegedVoidException(() -> {
            if (blobContainer.exists()) {
-                for (ListBlobItem blobItem : blobContainer.listBlobs(keyPath + (prefix == null ? "" : prefix))) {
+                for (ListBlobItem blobItem : blobContainer.listBlobs(keyPath + (prefix==null ? "" : prefix),false,


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

rjernst · 2017-08-17T23:10:26Z

...tory-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java

    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


typo: an -> a, concat -> concatenated

rjernst · 2017-08-17T23:10:40Z

...tory-azure/src/test/java/org/elasticsearch/repositories/azure/AzureSnapshotRestoreTests.java

-        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
+         * (can't have 2 consecutives hypens on Azure containers)


typo: hypens -> hyphens

etiennecarriere · 2017-08-18T08:58:02Z

Changes made. For the resolution of the conflicts, I don't know what is the Elasticsearch method :

Use the github resolver (as I have done)
Made a rebase then force push to the branch (I prepared it on my local repo).

rjernst · 2017-08-23T17:54:15Z

@elasticmachine test this please

rjernst · 2017-08-23T17:54:46Z

Thanks @Etienne-Carriere, I will get this merged once CI completes.

rjernst · 2017-08-23T20:48:24Z

@Etienne-Carriere This branch no longer compiles against master. Can you bring it up to date? I think the ctor for azure impl has been changed.

etiennecarriere · 2017-08-24T10:36:49Z

I change the API call . I made the following tests :
gradle assemble on the root directory : OK
gradle check on the plugins/repository-azure : OK

but the gradle check the root directory failed on an error that seems not to be linked with this PR.

rjernst · 2017-08-24T14:37:56Z

@elasticmachine please test this

etiennecarriere · 2017-09-07T10:16:48Z

@rjernst , do you need additionnals elements to validate the PR ?

rjernst

LGTM. Thanks for your patience @Etienne-Carriere

…pshot) (#25710) This commit reworks the azure listing of snapshot files to do a single listing, instead of once per blob. closes #25424

…rflow * origin/master: (59 commits) Fix Lucene version of 5.6.1. Remove azure deprecated settings (elastic#26099) Handle the 5.6.0 release Allow plugins to validate cluster-state on join (elastic#26595) Remove index mapper dynamic settings (elastic#25734) update AWS SDK for ECS Task IAM support in discovery-ec2 (elastic#26479) Azure repository: Accelerate the listing of files (used in delete snapshot) (elastic#25710) Build: Remove norelease from forbidden patterns (elastic#26592) Fix reference to painless inside expression engine (elastic#26528) Build: Move javadoc linking to root build.gradle (elastic#26529) Test: Remove leftover static bwc test case (elastic#26584) Docs: Remove remaining references to file and native scripts (elastic#26580) Snapshot fallback should consider build.snapshot elastic#26496: Set the correct bwc version after backport to 6.x Fix the MapperFieldType.rangeQuery API. (elastic#26552) Deduplicate `_field_names`. (elastic#26550) [Docs] Update method setSource(byte[] source) (elastic#26561) [Docs] Fix typo in javadocs (elastic#26556) Allow multiple digits in Vagrant 2.x minor versions Support Vagrant 2.x ...

* master: (21 commits) Ensure module is bundled before installing in tests Add boolean similarity to built in similarity types (elastic#26613) [Tests] Remove skip tests in search/30_limits.yml Let search phases override max concurrent requests Add a soft limit for the number of requested doc-value fields (elastic#26574) Support for accessing Azure repositories through a proxy (elastic#23518) Add beta tag to MSI Windows Installer (elastic#26616) Fix Lucene version of 5.6.1. Remove azure deprecated settings (elastic#26099) Handle the 5.6.0 release Allow plugins to validate cluster-state on join (elastic#26595) Remove index mapper dynamic settings (elastic#25734) update AWS SDK for ECS Task IAM support in discovery-ec2 (elastic#26479) Azure repository: Accelerate the listing of files (used in delete snapshot) (elastic#25710) Build: Remove norelease from forbidden patterns (elastic#26592) Fix reference to painless inside expression engine (elastic#26528) Build: Move javadoc linking to root build.gradle (elastic#26529) Test: Remove leftover static bwc test case (elastic#26584) Docs: Remove remaining references to file and native scripts (elastic#26580) Snapshot fallback should consider build.snapshot ...

etiennecarriere added 2 commits July 13, 2017 17:29

Update Integrations tests

081f0e0

Update the ListBlob in Container in order to optimize the network re…

f676649

…quests

rjernst reviewed Jul 13, 2017

View reviewed changes

Corrections following the remarks on the pull request

e6478a6

etiennecarriere force-pushed the 25424_azure_improve_delete_snapshot branch from 6ad3fb6 to e6478a6 Compare July 14, 2017 02:28

Remarks taken in account in the integration test

9e1a0a1

dakrone requested a review from rjernst August 15, 2017 21:23

rjernst requested changes Aug 17, 2017

View reviewed changes

etiennecarriere added 2 commits August 18, 2017 10:43

Minor modifications on comments

3871628

Merge branch 'master' into 25424_azure_improve_delete_snapshot

8e0ba87

rjernst approved these changes Aug 23, 2017

View reviewed changes

Change to comply with the change of API

90e1290

rjernst approved these changes Sep 11, 2017

View reviewed changes

rjernst added >enhancement v6.0.0 v6.1.0 labels Sep 11, 2017

rjernst added v7.0.0 >bug :Plugin Repository Azure and removed >enhancement labels Sep 11, 2017

rjernst merged commit 7060672 into elastic:master Sep 11, 2017

rjernst pushed a commit that referenced this pull request Sep 11, 2017

Azure repository: Accelerate the listing of files (used in delete sna…

f5f706e

…pshot) (#25710) This commit reworks the azure listing of snapshot files to do a single listing, instead of once per blob. closes #25424

rjernst pushed a commit that referenced this pull request Sep 11, 2017

Azure repository: Accelerate the listing of files (used in delete sna…

c7d8eb9

…pshot) (#25710) This commit reworks the azure listing of snapshot files to do a single listing, instead of once per blob. closes #25424

colings86 added v6.0.0-rc1 and removed v6.0.0 labels Sep 22, 2017

lcawl removed the v6.1.0 label Dec 12, 2017

clintongormley added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository Azure labels Feb 14, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

etiennecarriere commented Jul 13, 2017 •

edited

Loading

elasticmachine commented Jul 13, 2017

etiennecarriere commented Jul 13, 2017

rjernst left a comment

rjernst Jul 13, 2017

rjernst Jul 13, 2017

Etienne-Carriere Jul 13, 2017

abeyad Jul 14, 2017

rjernst Jul 13, 2017

rjernst Jul 13, 2017

etiennecarriere Jul 14, 2017

rjernst Jul 13, 2017

etiennecarriere Jul 14, 2017

rjernst commented Jul 13, 2017

karmi commented Jul 14, 2017

etiennecarriere commented Jul 14, 2017

etiennecarriere commented Jul 14, 2017

karmi commented Jul 17, 2017

rjernst left a comment

rjernst Aug 17, 2017

rjernst Aug 17, 2017

rjernst Aug 17, 2017

etiennecarriere commented Aug 18, 2017

rjernst commented Aug 23, 2017

rjernst commented Aug 23, 2017

rjernst commented Aug 23, 2017

etiennecarriere commented Aug 24, 2017

rjernst commented Aug 24, 2017

etiennecarriere commented Sep 7, 2017 •

edited

Loading

rjernst left a comment

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

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

Conversation

etiennecarriere commented Jul 13, 2017 • edited Loading

elasticmachine commented Jul 13, 2017

etiennecarriere commented Jul 13, 2017

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Jul 13, 2017

karmi commented Jul 14, 2017

etiennecarriere commented Jul 14, 2017

etiennecarriere commented Jul 14, 2017

karmi commented Jul 17, 2017

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etiennecarriere commented Aug 18, 2017

rjernst commented Aug 23, 2017

rjernst commented Aug 23, 2017

rjernst commented Aug 23, 2017

etiennecarriere commented Aug 24, 2017

rjernst commented Aug 24, 2017

etiennecarriere commented Sep 7, 2017 • edited Loading

rjernst left a comment

Choose a reason for hiding this comment

etiennecarriere commented Jul 13, 2017 •

edited

Loading

etiennecarriere commented Sep 7, 2017 •

edited

Loading