Storage queue linting, builder refactor, tests #4383

sima-zhu · 2019-07-10T23:35:34Z

Implements #4037

Implements Storage Queues with unit tests, JavaDocs, samples, README, and linting enabled.

alzimmermsft

I think the jacoco pom file needs to be updated to include Storage Queues

storage/client/queue/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java

storage/client/queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

storage/client/queue/src/main/java/com/azure/storage/queue/QueueClient.java

storage/client/queue/src/main/java/com/azure/storage/queue/QueueClientBuilder.java

storage/client/queue/src/main/java/com/azure/storage/queue/QueueServiceAsyncClient.java

storage/client/queue/src/main/java/com/azure/storage/queue/QueueServiceClientBuilder.java

storage/client/queue/src/main/java/com/azure/storage/queue/models/DequeuedMessage.java

storage/client/queue/src/samples/java/com/azure/storage/queue/AsyncSamples.java

storage/client/queue/src/samples/java/com/azure/storage/queue/MessageSample.java

sima-zhu · 2019-07-11T16:56:42Z

I think the jacoco pom file needs to be updated to include Storage Queues

@alzimmermsft Added storage queue as a dependency

sima-zhu · 2019-07-16T23:20:09Z

setAccessPolicy changing to varargs and Iterable overload
In JavaDocs explain the parameters in order of the argument list
DequeueMessages and PeekMessages should have their empty parameter overloads removed
Potentially changing the return type of getAccessPolicy to Response<List>
StorageException wrapper in postProcessResponse

jaschrep-msft · 2019-07-16T18:20:40Z

storage/client/queue/README.md

@@ -221,7 +220,7 @@ queueServiceClient.listQueuesSegment(marker, options).forEach{
 Get queue properties in account, including properties for Storage Analytics and CORS (Cross-Origin Resource Sharing) rules.
 ```Java
 String queueServiceURL = String.format("https://%s.queue.core.windows.net/%s", accountName, sasToken)
-QueueServiceClient queueServiceClient = QueueServiceClient.builder().endpoint(queueURL).build();
+QueueServiceClient queueServiceClient = new QueueServiceClientBuilder().endpoint(queueURL).build();


Catching up:
Didn't we want people to use the static method on the client to get the builder? Are we removing those now, or do we just want to make the sample look less cluttered?

The final design of how people instantiate clients is now as shown above - there are no longer .builder() methods on the individual client classes, instead users are directed to using the builder class directly. It saves in confusion where a user may do something like QueueServiceClient.builder().endpoint(...).buildAsyncClient() and wonder slightly why they started with a sync client class but ended up with an async client. CheckStyle rules are in development to ensure consistency.

Thanks @JonathanGiles

jaschrep-msft · 2019-07-16T20:27:52Z

storage/client/queue/src/main/java/com/azure/storage/queue/QueueClientBuilder.java

+    private URL endpoint;
+    private String queueName;
+    private SASTokenCredential sasTokenCredential;
+    private SharedKeyCredential sharedKeyCredential;


We need a TokenCredential option, like the blob service has.

What was the tokenCredential used for in queue?

jaschrep-msft · 2019-07-16T22:22:19Z

storage/client/queue/src/main/java/com/azure/storage/queue/README.md

+To create a Storage Account you can use the Azure Portal or [Azure CLI][azure_cli].
+
+```Powershell
+az group create \


What are the backslashes for? Maybe just a syntax mistake? Because this differs from your other multiline powershell snippits.

It is used for multi-line commands in bash. I checked other client library, they did not have the multi-line format. I will change it to one line command.

jaschrep-msft · 2019-07-17T17:28:35Z

storage/client/queue/src/main/java/com/azure/storage/queue/README.md

+### QueueClient
+Once you have the value of the SASToken you can create the queue service client with `${accountName}`, `${queueName}`, `${sasToken}`.
+```Java
+String queueURL = String.format("https://%s.queue.core.windows.net/%s%s", accountName, queueName, sasToken);


General note:
We have a builder specifically so people don't have to engineer their URLs, and then all our samples are us engineering URLs. I think it's important to show that if you already have a full URL, you can just use it. But we should be showing how to use our builders in these samples instead of doing string manipulation.

That's what we have several weeks ago. Will illustrate how to build clients in another example. And use the builder one for all the rest code examples.

JonathanGiles

An early pass on a few of the files in this PR. It would be wise to get this codebase updated to the latest Azure Core version. What is the plan there?

JonathanGiles · 2019-07-17T23:59:51Z

eng/jacoco-test-coverage/pom.xml

@@ -31,6 +31,7 @@
    <azure-identity.version>1.0.0-preview.1</azure-identity.version>
    <azure-keyvault.version>4.0.0-preview.1</azure-keyvault.version>
    <azure-messaging-eventhubs.version>5.0.0-preview.2</azure-messaging-eventhubs.version>
+    <azure-storage-queue.version>12.0.0-preview.1</azure-storage-queue.version>


nit: these should go in alphabetical order, so 'queue' comes after 'blob'.

JonathanGiles · 2019-07-18T00:01:03Z

eng/jacoco-test-coverage/pom.xml

+      <groupId>com.azure</groupId>
+      <artifactId>azure-storage-queue</artifactId>
+      <version>${azure-storage-queue.version}</version>
+    </dependency>


Alphabetical here (based on artifactId) - push to after blob below.

eng/spotbugs-aggregate-report/pom.xml

JonathanGiles · 2019-07-18T00:01:52Z

eng/spotbugs-aggregate-report/pom.xml

+          <groupId>com.azure</groupId>
+          <artifactId>azure-storage-queue</artifactId>
+          <version>${azure-storage-queue.version}</version>
+        </dependency>


pom.client.xml

JonathanGiles · 2019-07-18T00:11:40Z

storage/client/queue/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java

+     *                                  <ul>
+     *                                      <li> There is only one null value for retryDelay and maxRetryDelay.<li/>
+     *                                      <li> Unrecognized retry policy type.<li/>
+     *                                  </ul>


This is better - but the indentation is unnecessary.

I aligned with the start point of the description. Follow the format above. Could you explain it more where these line should be?

Move it all the way to the left with just a space (or a few of them at most).

storage/client/queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

JonathanGiles · 2019-07-18T00:20:22Z

storage/client/queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

+    private final String queueName;
+
+    /**
+     * Creates a QueueAsyncClient that sends requests to the storage queue service at {@link AzureQueueStorageImpl#url() endpoint}.


This is not a public method, so it is not visible to users. However, please validate all JavaDoc for issues. For example, here AzureQueueStorageImpl is not public API, so it should not be referred to in JavaDoc.

Will remove the link here. And review for the rest.

JonathanGiles · 2019-07-18T00:22:24Z

storage/client/queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

+        try {
+            return new URL(client.url());
+        } catch (MalformedURLException ex) {
+            throw new RuntimeException("Queue URL is malformed");


ClientLogger.logAndThrow all exceptions. Please search for all cases because soon Shawn will check in his CheckStyle rule and it will break the build.

Do you mean I added a log message before throw statement. Done.

No. Just call Logger.logAndThrow(new RuntimeException("....") and then return null afterwards.

JonathanGiles · 2019-07-18T00:23:02Z

storage/client/queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

+
+    /**
+     * @return the URL of the storage queue
+     * @throws RuntimeException If the queue is using a malformed URL.


Is there ever a way for this to be malformed? If it was we would have identified it in the builder before this class was ever created.

I don't think it has a chance of being thrown, but It is a checked exception. It needs to be handled either in try/catch or in signature. Alan and I both think it is better to do try/catch block.

What is your suggestion about handling the exception?

The other option is to swallow the exception

sima-zhu · 2019-07-18T21:52:05Z

An early pass on a few of the files in this PR. It would be wise to get this codebase updated to the latest Azure Core version. What is the plan there?

Queue, file will have the same schedule with Blob to move to core preview 3. @alzimmermsft suggested to put it to storage preview 3. @alzimmermsft please correct me if you have anything else on top of your plan.

sima-zhu · 2019-07-20T00:15:58Z

James and Jonathan agreed to merge in the PR and addressed all issues in Epic for next round.

jaschrep-msft

Missing items have been agreed to be addressed in a later PR.

JonathanGiles · 2019-07-22T01:34:12Z

storage/client/queue/README.md

-String queueServiceURL = String.format("https://%s.queue.core.windows.net/%s", accountName, sasToken)
-QueueServiceClient queueServiceClient = QueueServiceClient.builder().endpoint(queueURL).build();
+String queueServiceURL = String.format("https://%s.queue.core.windows.net", accountName)
+QueueServiceClient queueServiceClient = new QueueServiceClientBuilder().endpoint(queueURL).credential(SASToken).build();


If I recall, the QueueServiceClientBuilder class is either being renamed or has been renamed?

I did not realize we need to rename the builder for this PR. There is a proposal to rename the QueueServiceClient -> QueueStorageClient.

I will address the rename in issue #4532

JonathanGiles · 2019-07-22T02:14:48Z

storage/client/queue/pom.xml

+
+  <groupId>com.azure</groupId>
+  <artifactId>azure-storage-queue</artifactId>
+  <version>12.0.0-preview.1</version>


Is queues going to release as preview 1?

@alzimmermsft said we can have the version number 12.0.0-preview.1

I did not aware of any other proposal.

Let me know if we need to change to something else.

This will be changed to Preview 2, File, Queues, and the Common package will all skip Preview 1 to be consistent with Blobs and the other languages.

JonathanGiles · 2019-07-22T02:16:00Z

storage/client/queue/pom.xml

+    <dependency>
+      <groupId>org.slf4j</groupId>
+      <artifactId>slf4j-api</artifactId>
+    </dependency>


slf4j shouldn't be required here.

JonathanGiles · 2019-07-22T02:16:17Z

storage/client/queue/pom.xml

+      <groupId>org.slf4j</groupId>
+      <artifactId>slf4j-simple</artifactId>
+      <scope>test</scope>
+    </dependency>


JonathanGiles · 2019-07-22T02:16:37Z

storage/client/queue/pom.xml

+      <groupId>com.microsoft.azure</groupId>
+      <artifactId>adal4j</artifactId>
+      <scope>test</scope>
+    </dependency>


Is reactor-test or adal4j used?

Removed adal4j

Reator-test is necessary for async tests.

JonathanGiles · 2019-07-22T02:18:39Z

...age/client/queue/src/main/java/com/azure/storage/common/credentials/SharedKeyCredential.java

-     * @throws InvalidKeyException If the accountKey is not a valid Base64-encoded string.
+     * @throws RuntimeException for one of the following cases:
+     *                              - If the HMAC-SHA256 signature for {@code sharedKeyCredentials} fails to generate.
+     *                              - If the an invalid key has been given to the client.


The javadoc should be fixed before it is merged - it is just a formatting issue, not a logic issue.

JonathanGiles · 2019-07-22T02:29:30Z

...age/client/queue/src/main/java/com/azure/storage/common/credentials/SharedKeyCredential.java

+        } catch (final NoSuchAlgorithmException e) {
+            throw new RuntimeException(e);
+        } catch (InvalidKeyException e) {
+            throw new RuntimeException("Please double check the account key. Error details: " + e.getMessage());


No, this is not correct. Use logAndThrow

JonathanGiles · 2019-07-22T02:33:12Z

storage/client/queue/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java

+     *                                  <ul>
+     *                                      <li> There is only one null value for retryDelay and maxRetryDelay.<li/>
+     *                                      <li> Unrecognized retry policy type.<li/>
+     *                                  </ul>


Move it all the way to the left with just a space (or a few of them at most).

JonathanGiles · 2019-07-22T02:34:26Z

storage/client/queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

+
+    /**
+     * @return the URL of the storage queue
+     * @throws RuntimeException If the queue is using a malformed URL.


The other option is to swallow the exception

JonathanGiles · 2019-07-22T02:35:16Z

storage/client/queue/src/main/java/com/azure/storage/queue/QueueAsyncClient.java

+        try {
+            return new URL(client.url());
+        } catch (MalformedURLException ex) {
+            throw new RuntimeException("Queue URL is malformed");


No. Just call Logger.logAndThrow(new RuntimeException("....") and then return null afterwards.

JonathanGiles · 2019-07-23T05:01:16Z

I gave feedback that has not been addressed, yet this PR was merged. Were issues filed or have these been fixed? I'm not super keen to have PRs merged when people are still discussing it (regardless of it is myself or anyone else).

sima-zhu

I gave feedback that has not been addressed, yet this PR was merged. Were issues filed or have these been fixed? I'm not super keen to have PRs merged when people are still discussing it (regardless of it is myself or anyone else).

I have opened the PR over the weekend. Did not realize you have new comments right here.
I have open another PR to address all the comments here.

Sorry for the confusion.

* Storage SAS implementation (#4404) * SAS implementation * Fixed some minor formatting issues * Fixed checkstyle problems and test issue * Remove RawClients from Blobs (#4375) Removes RawClients from Storage Blobs * Add deleteContainer to StorageClient and getBlobClient with Snapshot to ContainerClient (#4376) * Removed raw clients * Added deleteContainer to StorageClient * Added getAppendBlob with snapshot to ContainerClient * Storage queue linting, builder refactor, tests (#4383) * Initial check in for storage queue * Initial checkin for Storage file (#4414) * Finished the restructure, refactor builder. Added sleep in record feature. Linting * Merge Storage Blob Client Builders (#4468) Merges AppendBlobClientBuilder, BlobClientBuilder, BlockBlobClientBuilder, and PageBlobClientBuilder into a single builder class BlobClientBuilder. Additionally, JavaDoc comments for the other builder classes, ContainerClientBuilder and StorageAccountClientBuilder, were cleaned up and the way the endpoint is handled in builders was changed.

* Storage SAS implementation (#4404) * SAS implementation * Fixed some minor formatting issues * Fixed checkstyle problems and test issue * Remove RawClients from Blobs (#4375) Removes RawClients from Storage Blobs * Add storage swaggers * Update Storage to azure-core preview 3 * Add deleteContainer to StorageClient and getBlobClient with Snapshot to ContainerClient (#4376) * Removed raw clients * Added deleteContainer to StorageClient * Added getAppendBlob with snapshot to ContainerClient * Storage queue linting, builder refactor, tests (#4383) * Initial check in for storage queue * Address code review feedback in AutoRest * Initial checkin for Storage file (#4414) * Finished the restructure, refactor builder. Added sleep in record feature. Linting * Merge Storage Blob Client Builders (#4468) Merges AppendBlobClientBuilder, BlobClientBuilder, BlockBlobClientBuilder, and PageBlobClientBuilder into a single builder class BlobClientBuilder. Additionally, JavaDoc comments for the other builder classes, ContainerClientBuilder and StorageAccountClientBuilder, were cleaned up and the way the endpoint is handled in builders was changed. * Regenerate blob file queue after merge * Up azure core version for file & queue * Apply all the manual fixes in swagger * Change directoryName to directoryPath * Use non-fluent pattern for generated clients * Fix build after merging with master * Return void on setters * Revert spotbugs config change

* Storage SAS implementation (#4404) * SAS implementation * Fixed some minor formatting issues * Fixed checkstyle problems and test issue * Remove RawClients from Blobs (#4375) Removes RawClients from Storage Blobs * Add deleteContainer to StorageClient and getBlobClient with Snapshot to ContainerClient (#4376) * Removed raw clients * Added deleteContainer to StorageClient * Added getAppendBlob with snapshot to ContainerClient * Storage queue linting, builder refactor, tests (#4383) * Initial check in for storage queue * Beginning of code snippets for BlobAsyncClient * JavaDoc snippets for BlobAsyncClient and BlobClient * Added links to REST API docs for BlobAsyncClient and BlobClient, fixed JavaDoc issues, fix unit test issues * Removed invalid import

sima-zhu added 2 commits July 10, 2019 16:27

Linting, builder refactor and playback tests

bf6a460

Remove unnecssary changes

a5a8a87

sima-zhu requested a review from alzimmermsft July 10, 2019 23:37

praries880 closed this Jul 11, 2019

praries880 reopened this Jul 11, 2019

alzimmermsft requested changes Jul 11, 2019

View reviewed changes

Address Alan's feedback.

bc590b5

sima-zhu requested a review from alzimmermsft July 11, 2019 18:54

sima-zhu added 2 commits July 11, 2019 12:12

Remove unused fields

52a8de0

Remove unused imports

4205b7e

joshfree assigned sima-zhu Jul 12, 2019

alzimmermsft approved these changes Jul 12, 2019

View reviewed changes

sima-zhu added 3 commits July 12, 2019 23:42

Merge branch 'storage-post-review1-dev' into storage_queue

d3de0c3

Fixed the missing tag

4091695

Fixed the missing group id

433dfbe

alzimmermsft requested a review from JonathanGiles July 15, 2019 21:20

sima-zhu requested review from rickle-msft and jaschrep-msft July 15, 2019 21:28

sima-zhu added 5 commits July 15, 2019 15:26

Added the queue service javadoc code snippet.

45e5fa4

Added the queue service builder javadoc code snippet.

1338dc7

Added the queue builder javadoc code snippet.

844c0ff

Queue client java doc code snippet

1369721

Make minor changes and fix checkstyle.

9db31b9

jaschrep-msft reviewed Jul 17, 2019

View reviewed changes

alzimmermsft added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jul 17, 2019

JonathanGiles requested changes Jul 18, 2019

View reviewed changes

Addressed comments.

4788a13

sima-zhu requested review from jaschrep-msft and JonathanGiles July 18, 2019 21:41

Make changes to queue configuration

73f0b89

rickle-msft approved these changes Jul 19, 2019

View reviewed changes

Minor changes for queue java doc and readme code snippet.

5b540c6

sima-zhu requested review from danieljurek, mitchdenny and weshaggard as code owners July 20, 2019 00:15

fix checkstyle

c440e58

jaschrep-msft approved these changes Jul 22, 2019

View reviewed changes

JonathanGiles requested changes Jul 22, 2019

View reviewed changes

sima-zhu merged commit 78ea66b into Azure:storage-post-review1-dev Jul 22, 2019

sima-zhu commented Jul 23, 2019

View reviewed changes

sima-zhu mentioned this pull request Jul 23, 2019

Changes for javadoc and logger #4552

Closed

Storage queue linting, builder refactor, tests #4383

Storage queue linting, builder refactor, tests #4383

Conversation

sima-zhu commented Jul 10, 2019 • edited by alzimmermsft Loading

alzimmermsft left a comment

Choose a reason for hiding this comment

sima-zhu commented Jul 11, 2019

sima-zhu commented Jul 16, 2019

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

JonathanGiles 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

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

sima-zhu commented Jul 18, 2019

sima-zhu commented Jul 20, 2019

jaschrep-msft 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

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

JonathanGiles commented Jul 23, 2019

sima-zhu left a comment

Choose a reason for hiding this comment

sima-zhu commented Jul 10, 2019 •

edited by alzimmermsft

Loading