CR feedback and updated Contributing doc

Author: rickle-msft

CONTRIBUTING.md

@@ -20,7 +20,11 @@ The Azure Storage development team uses Intellij. However, any preferred IDE or
 ## Tests
 
 ### Configuration
-The only step to configure testing is to set the appropriate environment variable. Create environment variables named "ACCOUNT_NAME", "ACCOUNT_KEY", "SECONDARY_ACCOUNT_NAME", and "SECONDARY_ACCOUNT_KEY". The first two will be used for most requests. The second two will only be used for tests requiring two accounts.
+The only step to configure testing is to set the appropriate environment variables. Create environment variables named "ACCOUNT_NAME" and "ACCOUNT_KEY", holding your Azure storage account name and key respectively. This will satisfy most tests. 
+To run any tests requiring two accounts (generally those testing copy-related apis), set environment variables "SECONDARY_ACCOUNT_NAME", and "SECONDARY_ACCOUNT_KEY".
+To run any tests related to setting blob tiers on block blobs, set environment variables "BLOB_STORAGE_ACCOUNT_NAME" and "BLOB_STORAGE_ACCOUNT_KEY". Note that a GPV2 account is also sufficient here.
+To run any tests related to setting blob tiers on page blobs, set environment variables "PREMIUM_ACCOUNT_NAME" and "PREMIUM_ACCOUNT_KEY".
+It is valid to use a single account for multiple scenarios; a GPV2 account would work for both the primary account and the blob storage account, for instance. The only restriction is that the primary and secondary accounts must be distinct.
 
 ### Running
 To actually run tests, right click on the test class in the Package Explorer or the individual test in the Outline and select Run As->GroovyTest. Alternatively, run mvn test from the command line.

ChangeLog.txt

@@ -5,6 +5,8 @@ XXXX.XX.XX Version XX.X.X
 * Added support for IProgressReceiver in TransferManager operations. This parameter was previously ignored but is now supported.
 * Removed internal dependency on javafx to be compatible with openjdk.
 * Fixed a bug that would cause downloading large files with the TransferManager to fail and a bug in BlobURL.download() logic for setting up reliable download.
+* Fixed a bug that would cause downloading large files with the TransferManager to fail.
+* Fixed a bug in BlobURL.download() logic for setting up reliable download. This had the potential to download the wrong range when a download stream was retried.
 
 2018.09.11 Version 10.1.0
 * Interfaces for helper types updated to be more consistent throughout the library. All types, with the exception of the options for pipeline factories, use a fluent pattern.

src/main/java/com/microsoft/azure/storage/blob/DownloadResponse.java

@@ -86,6 +86,12 @@ private Flowable<ByteBuffer> tryContinueFlowable(Throwable t, int retryCount, Re
         if (retryCount > options.maxRetryRequests() || !(t instanceof IOException)) {
             return Flowable.error(t);
         } else {
+            /*
+            We wrap this in a try catch because we don't know the behavior of the getter. Most errors would probably
+            come from an unsuccessful request, which would be propagated through the onError methods. However, it is
+            possible the method call that returns a Single is what throws (like how our apis throw some exceptions at
+            call time rather than at subscription time.
+             */
             try {
                 // Get a new response and try reading from it.
                 return getter.apply(this.info)

src/main/java/com/microsoft/azure/storage/blob/HTTPGetterInfo.java

@@ -55,7 +55,7 @@ public Long count() {
      */
     public HTTPGetterInfo withCount(Long count) {
         if (count != null) {
-            Utility.assertInBounds("count", count, 0, Integer.MAX_VALUE);
+            Utility.assertInBounds("count", count, 0, Long.MAX_VALUE);
         }
         this.count = count;
         return this;

src/main/java/com/microsoft/azure/storage/blob/RequestRetryOptions.java

@@ -84,7 +84,7 @@ public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, In
         }
 
         if (tryTimeout != null) {
-            Utility.assertInBounds("tryTimeoutInMs", tryTimeout, 1, Long.MAX_VALUE);
+            Utility.assertInBounds("tryTimeout", tryTimeout, 1, Integer.MAX_VALUE);
             this.tryTimeout = tryTimeout;
         } else {
             this.tryTimeout = 60;

src/test/java/com/microsoft/azure/storage/APISpec.groovy

@@ -229,25 +229,24 @@ class APISpec extends Specification {
         }
     }
 
+    /*
+    Size must be an int because ByteBuffer sizes can only be an int. Long is not supported.
+     */
     static ByteBuffer getRandomData(int size) {
         Random rand = new Random(getRandomSeed())
         byte[] data = new byte[size]
         rand.nextBytes(data)
         return ByteBuffer.wrap(data)
     }
 
-    static File getRandomFile(long size) {
+    /*
+    We only allow int because anything larger than 2GB (which would require a long) is left to stress/perf.
+     */
+    static File getRandomFile(int size) {
         File file = File.createTempFile(UUID.randomUUID().toString(), ".txt")
         file.deleteOnExit()
         FileOutputStream fos = new FileOutputStream(file)
-        Random rand = new Random(getRandomSeed())
-        while (size > 0) {
-            int sizeToWrite = (int) Math.min((long)(Integer.MAX_VALUE.longValue()/10L), size)
-            byte[] data = new byte[sizeToWrite]
-            rand.nextBytes(data)
-            fos.write(data)
-            size -= sizeToWrite
-        }
+        fos.write(getRandomData(size).array())
         fos.close()
         return file
     }

src/test/java/com/microsoft/azure/storage/BlobAPITest.groovy

@@ -89,7 +89,7 @@ class BlobAPITest extends APISpec {
         setup:
         def mockPolicy = Mock(RequestPolicy) {
             sendAsync(_) >> { HttpRequest request ->
-                if (request.headers().value("x-ms-range") != "bytes=2-") {
+                if (request.headers().value("x-ms-range") != "bytes=2-6") {
                     return Single.error(new IllegalArgumentException("The range header was not set correctly on retry."))
                 }
                 else {
@@ -102,7 +102,7 @@ class BlobAPITest extends APISpec {
         bu = bu.withPipeline(pipeline)
 
         when:
-        def range = new BlobRange().withOffset(2)
+        def range = new BlobRange().withOffset(2).withCount(5)
         bu.download(range, null, false, null).blockingGet().body(new ReliableDownloadOptions().withMaxRetryRequests(3))
                 .blockingSubscribe()
 

src/test/java/com/microsoft/azure/storage/TransferManagerTest.groovy

@@ -424,7 +424,7 @@ class TransferManagerTest extends APISpec {
         file                                   | _
         getRandomFile(20)                      | _ // small file
         getRandomFile(16 * 1024 * 1024)        | _ // medium file in several chunks
-        getRandomFile(8L * 1026 * 1024 + 10)   | _ // medium file not aligned to block
+        getRandomFile(8 * 1026 * 1024 + 10)   | _ // medium file not aligned to block
         getRandomFile(0)                       | _ // empty file
         // Files larger than 2GB to test no integer overflow are left to stress/perf tests to keep test passes short.
     }