fixes for enforce final fields checkstyle rule - Azure Storage #5010

vhvb1989 · 2019-08-15T18:14:22Z

No description provided.

* Move Storage to official swagger files There are no (meaningful) code changes except the attribute in BlobHierarchyListSegment that I can't trace to the swagger * Upgrade blobs swagger to 2019-02-02 Note that additional work will have to be done to patch our higher level code to pass new parameters to existing APIs * Fixes to Swagger generation and updating to use new protocol layer * Fixed appendBlock content-type, added SpotBug exclusions for autogen code * Update README.md * Fixed incorrect exclude

* Migrate queue to spock test framework * Remove all Junit tests and dependencies

conniey · 2019-08-15T21:35:34Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobOutputStream.java

-        }
-
-        this.internalWriteThreshold = (int) BlockBlobAsyncClient.BLOB_DEFAULT_UPLOAD_BLOCK_SIZE;
+        this(parentBlob, accessCondition != null ? new BlobAccessConditions().modifiedAccessConditions(


This is hard to discern. I wouldn't add the constructor above.

yeah, this was one of the most annoying classes to fix XD

Constructor sets fields based on multiple conditions.
I added the private constructor to make sure all fields are always assigned so fields can be final without duplicating the code in each constructor.

The other option is to add private method that set fields but that would make fields not final

Options here are:

Update code to make fields not final by adding private methods to set fields. This will simplify constructor logic

Add some code duplicity for assigning fields in two constructors to enhance readability for this constructor.

Avoid code duplicity by adding hard-to-discern calls (like it is right now)

Create a ticket and assign to some code owner to decide what he/she wants to do and assign a suppress rule for it

Any other idea is welcome

what do you think @conniey

This makes it hard to read. There are several ternary operators here and not clear what it's doing. Given that it's calling another overloaded constructor, you can't have any other statements before calling this() either. So, I would lean towards making it readable/maintainable even if that results in a tiny bit of code duplication.

This could be a case to break this down into four classes, one which handles the interface/abstract methods, and three more which implement that for each individual use case.

Taking number #4 after talking to Conniey.
Tiket created: #5021

Rule to be supressed in this file until issue is fixed

I made a mistake while trying to fix this PR and had it all wrong.
PR got closed after deleting branch from origin
I created a new one as this one should have been #5022

JonathanGiles

LGTM, ignoring the issue Connie raises. Once that is fixed to her satisfaction, this can be merged.

JonathanGiles · 2019-08-16T01:45:03Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobOutputStream.java

-        this.streamType = BlobType.BLOCK_BLOB;
-        this.internalWriteThreshold = (int) BlockBlobAsyncClient.BLOB_DEFAULT_UPLOAD_BLOCK_SIZE;
+        this(parentBlob, accessCondition, null, UUID.randomUUID().toString() + "-", new TreeMap<>(), 0,
+            BlobType.BLOCK_BLOB, BlockBlobAsyncClient.BLOB_DEFAULT_UPLOAD_BLOCK_SIZE);


The last argument is BlockBlobAsyncClient.BLOB_DEFAULT_UPLOAD_BLOCK_SIZE, but the argument name is internalWriteThreshold. I can see this is what used to be there, but it is weird naming? // @alzimmermsft

It's the threshold used until another page, block, etc is requested. Maybe this could be renamed to bufferSize? Could just remove the internal and have writeThreshold.

srnagar · 2019-08-16T01:50:10Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobOutputStream.java

-//        .doOnComplete(() -> completionSink.complete());
+    private BlobOutputStream(final BlobAsyncClient blobClient, final BlobAccessConditions accessCondition,
+                             final AppendPositionAccessConditions appendPositionAccessConditions,
+                             final String blockIdPrefix, final TreeMap<Long, String> blockList,


Does it have to be a TreeMap? Can a Map be used instead?

@jianghaolu was TreeMap used to maintain some ordering?

srnagar · 2019-08-16T01:56:20Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobOutputStream.java


    /**
     * Used for block blobs, holds the block list.
     */
-    private TreeMap<Long, String> blockList;
+    private final TreeMap<Long, String> blockList;


It's not related to this PR but curious if Map can be used instead? Also, why is it called a list?

It's called blockList given that it is used by BlockBlob's Put Blob List method. Given that this is the property on the class this can be changed to Map.

How do we like to handle this? Do you want me to update the name as part of this same PR? or should we create a new issue to request this change to have an independent commit for that refactoring?

Issue created. Since there is more things to address than just names or type updating
#5021

srnagar · 2019-08-16T02:04:43Z

sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobOutputStream.java

-        }
-
-        this.internalWriteThreshold = (int) BlockBlobAsyncClient.BLOB_DEFAULT_UPLOAD_BLOCK_SIZE;
+        this(parentBlob, accessCondition != null ? new BlobAccessConditions().modifiedAccessConditions(


This makes it hard to read. There are several ternary operators here and not clear what it's doing. Given that it's calling another overloaded constructor, you can't have any other statements before calling this() either. So, I would lean towards making it readable/maintainable even if that results in a tiny bit of code duplication.

alzimmermsft

LGTM, just need to resolve the questions.

…literals usage (#4994) * Adding javaDoc formating rule * Adding fixes for javadoc formatting

fixes for enforce final fields checkstyle rule

3d38080

vhvb1989 requested review from JonathanGiles, conniey, mssfang and alzimmermsft August 15, 2019 18:14

vhvb1989 requested review from jaschrep-msft, rickle-msft and sima-zhu as code owners August 15, 2019 18:14

This was referenced Aug 15, 2019

Adding rule for enforcing final field usage - DoNotMerge until all issues are fixed #5005

Merged

Check style final fields if assigned in constructor only #4854

Closed

tg-msft and others added 2 commits August 15, 2019 11:40

adding fixes missing for azure storage project for final fields check

0ff8e75

vhvb1989 self-assigned this Aug 15, 2019

vhvb1989 added the Java Source Code Rules label Aug 15, 2019

Added spock test framework and tests for queue service (#4940)

6eb9e89

* Migrate queue to spock test framework * Remove all Junit tests and dependencies

conniey reviewed Aug 15, 2019

View reviewed changes

vhvb1989 requested a review from conniey August 15, 2019 22:10

mitchdenny and others added 2 commits August 15, 2019 17:27

Fix directory path. (#5012)

2eec622

Renaming IterableResponse to IterableStream (#5002)

56dc993

JonathanGiles approved these changes Aug 16, 2019

View reviewed changes

srnagar reviewed Aug 16, 2019

View reviewed changes

Add tracing context to storage blobs (#4752)

faa0ef7

alzimmermsft approved these changes Aug 16, 2019

View reviewed changes

vhvb1989 and others added 3 commits August 16, 2019 10:15

Checkstyle - Javadoc - enforce formating for param, return and throw …

6f0f73f

…literals usage (#4994) * Adding javaDoc formating rule * Adding fixes for javadoc formatting

sync to master

1084cef

suppress final field rule for BlobOutputStream

d49ad5f

vhvb1989 requested review from danieljurek, g2vinay, hemanttanwar and jianghaolu as code owners August 16, 2019 18:32

vhvb1989 requested review from mitchdenny, samvaity and weshaggard as code owners August 16, 2019 18:32

vhvb1989 closed this Aug 16, 2019

vhvb1989 deleted the enforce-final-fields-azure-storage branch August 16, 2019 18:50

vhvb1989 mentioned this pull request Aug 16, 2019

adding final field rule fixes for azure storage #5022

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes for enforce final fields checkstyle rule - Azure Storage #5010

fixes for enforce final fields checkstyle rule - Azure Storage #5010

vhvb1989 commented Aug 15, 2019

conniey Aug 15, 2019

vhvb1989 Aug 15, 2019

srnagar Aug 16, 2019

alzimmermsft Aug 16, 2019

vhvb1989 Aug 16, 2019

vhvb1989 Aug 16, 2019

JonathanGiles left a comment

JonathanGiles Aug 16, 2019

alzimmermsft Aug 16, 2019

srnagar Aug 16, 2019

alzimmermsft Aug 16, 2019

srnagar Aug 16, 2019

alzimmermsft Aug 16, 2019

vhvb1989 Aug 16, 2019

vhvb1989 Aug 16, 2019 •

edited

Loading

srnagar Aug 16, 2019

alzimmermsft left a comment

fixes for enforce final fields checkstyle rule - Azure Storage #5010

fixes for enforce final fields checkstyle rule - Azure Storage #5010

Conversation

vhvb1989 commented Aug 15, 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

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

vhvb1989 Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzimmermsft left a comment

Choose a reason for hiding this comment

vhvb1989 Aug 16, 2019 •

edited

Loading