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.

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

Changes for javadoc and logger #4552

Closed
wants to merge 14 commits into from
Closed

Conversation

sima-zhu
Copy link
Contributor

@sima-zhu sima-zhu commented Jul 23, 2019

Addressed the missed feedback from last PR.
Removed extra README.

Last PR: #4383

gapra-msft and others added 8 commits July 12, 2019 11:57
* SAS implementation

* Fixed some minor formatting issues

* Fixed checkstyle problems and test issue
Removes RawClients from Storage Blobs
…to ContainerClient (Azure#4376)

* Removed raw clients

* Added deleteContainer to StorageClient

* Added getAppendBlob with snapshot to ContainerClient
@sima-zhu sima-zhu requested a review from JonathanGiles July 23, 2019 22:43
Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

Check if the asError logs should be asWarning. asError is meant to be used when the application would not be able to recover.

@sima-zhu sima-zhu requested a review from alzimmermsft July 23, 2019 23:18
@sima-zhu sima-zhu self-assigned this Jul 24, 2019
sima-zhu and others added 2 commits July 23, 2019 17:49
* Finished the restructure, refactor builder. Added sleep in record feature. Linting
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.
Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

Let's block this PR until #4489 merges in, most of the changes done here will be removed once it happens.

@sima-zhu sima-zhu changed the base branch from storage-post-review1-dev to master July 26, 2019 21:22
Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

This is the spec I am referring to: spec

@@ -236,8 +234,8 @@ private String computeHMACSHA256(String stringToSign) {
String signature = Base64.getEncoder().encodeToString(hmacSha256.doFinal(utf8Bytes));
return String.format(AUTHORIZATION_HEADER_FORMAT, accountName, signature);
} catch (NoSuchAlgorithmException | InvalidKeyException ex) {
logger.warning(ex.getMessage());
throw new Error(ex);
logger.logAndThrow(new RuntimeException(ex.getMessage()));
Copy link
Member

Choose a reason for hiding this comment

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

Pass in the exception itself into the constructor and change this to use IllegalStateException as that better aligns with the spec.

@@ -232,8 +234,8 @@ private String computeHMACSHA256(String stringToSign) {
String signature = Base64.getEncoder().encodeToString(hmacSha256.doFinal(utf8Bytes));
return String.format(AUTHORIZATION_HEADER_FORMAT, accountName, signature);
} catch (NoSuchAlgorithmException | InvalidKeyException ex) {
LOGGER.error(ex.getMessage());
throw new Error(ex);
logger.logAndThrow(new RuntimeException(ex.getMessage()));
Copy link
Member

Choose a reason for hiding this comment

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

Pass in the exception itself into the constructor and change this to use IllegalStateException as that better aligns with the spec.

String errorMsg = "There is no such algorithm. Error Details: " + e.getMessage();
logger.warning(errorMsg);
throw new RuntimeException(errorMsg);
logger.logAndThrow(new RuntimeException("There is no such algorithm. Error Details: " + e.getMessage()));
Copy link
Member

Choose a reason for hiding this comment

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

Change this to IllegalStateException as it better aligns with the spec.

String errorMsg = "Please double check the account key. Error details: " + e.getMessage();
logger.warning(errorMsg);
throw new RuntimeException(errorMsg);
logger.logAndThrow(new RuntimeException("Please double check the account key. Error details: " + e.getMessage()));
Copy link
Member

Choose a reason for hiding this comment

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

Change this to IllegalStateException as it better aligns with the spec.

@@ -116,11 +118,11 @@ public String computeHmac256(final String stringToSign) {
byte[] utf8Bytes = stringToSign.getBytes(StandardCharsets.UTF_8);
return Base64.getEncoder().encodeToString(hmacSha256.doFinal(utf8Bytes));
} catch (final NoSuchAlgorithmException e) {
LOGGER.error(e.getMessage());
throw new RuntimeException(e);
logger.logAndThrow(new RuntimeException(e));
Copy link
Member

Choose a reason for hiding this comment

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

Change this to IllegalStateException as it better aligns with the spec.

@@ -138,8 +138,8 @@ public QueueAsyncClient buildAsyncClient() {
Objects.requireNonNull(queueName);

if (sasTokenCredential == null && sharedKeyCredential == null) {
LOGGER.error("Credentials are required for authorization");
throw new IllegalArgumentException("Credentials are required for authorization");
logger.logAndThrow(new IllegalArgumentException("Credentials are required for authorization"));
Copy link
Member

Choose a reason for hiding this comment

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

Change this to IllegalStateException as it better aligns with the spec.

LOGGER.error("The Azure Storage Queue endpoint url is malformed. Endpoint: " + endpoint);
throw new IllegalArgumentException("The Azure Storage Queue endpoint url is malformed. Endpoint: " + endpoint);
logger.logAndThrow(new IllegalArgumentException("The Azure Storage Queue endpoint url is malformed. Endpoint: " + endpoint));
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to return null here, only use that when the code analyzer thinks there is no return case when one is needed.

@@ -71,8 +71,8 @@ public URL getQueueServiceUrl() {
try {
return new URL(client.getUrl());
} catch (MalformedURLException ex) {
LOGGER.error("Queue Service URL is malformed");
throw new RuntimeException("Storage account URL is malformed");
logger.logAndThrow(new RuntimeException("Storage account URL is malformed"));
Copy link
Member

Choose a reason for hiding this comment

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

Change this IllegalStateException to better align with the spec.

@@ -112,8 +112,8 @@ public QueueServiceAsyncClient buildAsyncClient() {
Objects.requireNonNull(endpoint);

if (sasTokenCredential == null && sharedKeyCredential == null) {
LOGGER.error("Credentials are required for authorization");
throw new IllegalArgumentException("Credentials are required for authorization");
logger.logAndThrow(new IllegalArgumentException("Credentials are required for authorization"));
Copy link
Member

Choose a reason for hiding this comment

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

Change this IllegalStateException to better align with the spec.

+ "Connection String: %s", connectionString);
throw new IllegalArgumentException(String.format("There is no valid account for the connection string. "
+ "Connection String: %s", connectionString));
logger.logAndThrow(new IllegalArgumentException(String.format("There is no valid account for the connection string. "
Copy link
Member

Choose a reason for hiding this comment

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

Change this IllegalStateException to better align with the spec.

@alzimmermsft alzimmermsft added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Aug 19, 2019
@sima-zhu
Copy link
Contributor Author

Close this and will address in issue:
#5066

@sima-zhu sima-zhu closed this Aug 20, 2019
@sima-zhu sima-zhu deleted the storage_queue branch February 23, 2021 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants