Skip to content

Conversation

@anfeldma-ms
Copy link
Contributor

No description provided.

@anfeldma-ms anfeldma-ms merged commit 30ca98a into master May 15, 2020
@anfeldma-ms anfeldma-ms deleted the dotNetParity branch May 15, 2020 12:28
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

great work Andy, some minor comments:

1- fix the code style, intellj has auto-format
2- use slf4j param place holder instead of string concat
3- use slf4j for exception logging rather than e.printStackTrace()

see this
String.format(


public void close() {
client.close();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please move the implementation methods like close() to the bottom of the sample.


// Database Create
private void createDatabaseIfNotExists() throws Exception {
logger.info("Create database " + databaseName + " if not exists...");
Copy link
Contributor

Choose a reason for hiding this comment

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

use slf4j placeholder rather than string concat


// Container create
private void createContainerIfNotExists() throws Exception {
logger.info("Create container " + containerName + " if not exists.");
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here and other places.

deleteADatabase();
} catch (Exception err) {
logger.error("Deleting Cosmos DB resources failed, will still attempt to close the client. See stack trace below.");
err.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

error needs to be passed to slf4j logger rather than directing to stderr'

Suggested change
err.printStackTrace();
logger.error("Deleting Cosmos DB resources failed, will still attempt to close the client. See stack trace below., e);

see this
http://www.slf4j.org/faq.html#paramException


private void autoscaleContainerCRUDDemo() throws Exception {

logger.info("Using Azure Cosmos DB endpoint: " + AccountSettings.HOST);
Copy link
Contributor

Choose a reason for hiding this comment

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

http://www.slf4j.org/faq.html#paramException
use slfrj place holder rather than string concat. here and other places.
http://www.slf4j.org/faq.html#paramException

logger.info("Demo complete, please hold while resources are released");
} catch (Exception e) {
e.printStackTrace();
logger.error(String.format("Cosmos getStarted failed with %s", e));
Copy link
Contributor

Choose a reason for hiding this comment

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

don't mix STring.format with slf4j logger
see this for how to do logging for error using slf4j
http://www.slf4j.org/faq.html#paramException

Suggested change
logger.error(String.format("Cosmos getStarted failed with %s", e));
logger.error("Cosmos getStarted failed", e);

p.autoscaleDatabaseCRUDDemo();
logger.info("Demo complete, please hold while resources are released");
} catch (Exception e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

remove print stacktrace in favour of passing e to logger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants