Reviewing Andy's change #7
Conversation
…sor Re: POJO vs JSON for inserting items
In SampleGroceryStore changefeed triggers three core functionalities
Migration guide
First run at Reactor RxJava guide
| | Java SDK | Release Date | Bundled APIs | Maven Jar | Java package name |API Reference | Release Notes | | ||
| |-------------------------|--------------|----------------------|-----------------------------------------|----------------------------------|-----------------------------------------------------------|------------------------------------------------------------------------------------------| | ||
| | Async 2.x.x | June 2018 | Async(RxJava) | com.microsoft.azure::azure-cosmosdb | com.microsoft.azure.cosmosdb.rx | [API](https://azure.github.io/azure-cosmosdb-java/2.0.0/) | [Release Notes](https://docs.microsoft.com/en-us/azure/cosmos-db/sql-api-sdk-async-java) | | ||
| | "Legacy" Sync 2.x.x | Sept 2018 | Sync | com.microsoft.azure::azure-documentdb | com.microsoft.azure.cosmosdb | [API](https://azure.github.io/azure-cosmosdb-java/2.0.0/) | [Release Notes](https://docs.microsoft.com/en-us/azure/cosmos-db/sql-api-sdk-java) | |
There was a problem hiding this comment.
legacy sync v2 package name is not com.microsoft.azure.cosmosdb. change to com.microsoft.azure.documentdb
| | Java SDK | Release Date | Bundled APIs | Maven Jar | Java package name |API Reference | Release Notes | | ||
| |-------------------------|--------------|----------------------|-----------------------------------------|----------------------------------|-----------------------------------------------------------|------------------------------------------------------------------------------------------| | ||
| | Async 2.x.x | June 2018 | Async(RxJava) | com.microsoft.azure::azure-cosmosdb | com.microsoft.azure.cosmosdb.rx | [API](https://azure.github.io/azure-cosmosdb-java/2.0.0/) | [Release Notes](https://docs.microsoft.com/en-us/azure/cosmos-db/sql-api-sdk-async-java) | | ||
| | "Legacy" Sync 2.x.x | Sept 2018 | Sync | com.microsoft.azure::azure-documentdb | com.microsoft.azure.cosmosdb | [API](https://azure.github.io/azure-cosmosdb-java/2.0.0/) | [Release Notes](https://docs.microsoft.com/en-us/azure/cosmos-db/sql-api-sdk-java) | |
There was a problem hiding this comment.
java sync 2.x package name is not cosmosdb. change to docmentdb instead.
| // Setting the preferred location to Cosmos DB Account region | ||
| defaultPolicy.setPreferredLocations(Lists.newArrayList("Your Account Location")); | ||
| // Use Direct Mode for best performance | ||
| defaultPolicy.setConnectionMode(ConnectionMode.DIRECT); |
There was a problem hiding this comment.
Direct is the default. you don't need to set it.
| }).flatMap(containerResponse -> { | ||
| container = containerResponse.getContainer(); | ||
| return Mono.empty(); | ||
| }).subscribe(); |
There was a problem hiding this comment.
in some places in doc you used block() and some places subscribe()
I would recommend we don't use subscribe() in general. as this is a simple guide, let's use block()
| }).flatMap(containerResponse -> { | ||
| container = containerResponse.container(); | ||
| return Mono.empty(); | ||
| }).subscribe(); |
There was a problem hiding this comment.
ditto on subscribe
|
|
||
| // ... | ||
|
|
||
| logger.info(String.format("Executing stored procedure %s...\n\n", sprocId)); |
There was a problem hiding this comment.
String.format is unnecessary. please use slf4j placeholder instead.
http://www.slf4j.org/manual.html#typical_usage
| .getStoredProcedure(sprocId) | ||
| .execute(null, options) | ||
| .flatMap(executeResponse -> { | ||
| logger.info(String.format("Stored procedure %s returned %s (HTTP %d), at cost %.3f RU.\n", |
There was a problem hiding this comment.
same here. we should use slfj4 placeholder remove String.fromat please.
| logger.info("----=>id: " + pojo_doc.getId()); | ||
|
|
||
| } catch (JsonProcessingException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
please use logger.error instead. no System.out or System.err
| //As a developer you have two options for handling the JsonNode document provided to you by Change Feed | ||
| //One option is to operate on the document in the form of a JsonNode, as shown below. This is great | ||
| //especially if you do not have a single uniform data model for all documents. | ||
| logger.info("---->DOCUMENT RECEIVED: " + OBJECT_MAPPER.writerWithDefaultPrettyPrinter() |
There was a problem hiding this comment.
please don't concat string. use slf4j place holder, here and elsewhere
| ```java | ||
| // Include a property that serializes to "ttl" in JSON | ||
| public class SalesOrder | ||
| { |
There was a problem hiding this comment.
please be consistent on the java code style.
"{" on the same line as class definition.
Update container
autoscale/analytical samples; DB/container CRUD; queries; template for usermanagement sample
moderakh
left a comment
moderakh
left a comment
There was a problem hiding this comment.
I added some minor comments.
also please fix code style, spacing, etc, intellj should have support for code style formatting.
|
|
||
| protected static Logger logger = LoggerFactory.getLogger(SampleChangeFeedProcessor.class.getSimpleName()); | ||
|
|
||
| public void close() { |
There was a problem hiding this comment.
I would move close() or any implementation method down the file and move the heart of the sample up in the file.
| p.containerCRUDDemo(); | ||
| logger.info("Demo complete, please hold while resources are released"); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
e.printStackTrace() is unnecessary as you are logging the exception using the logger next line.
| 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)); |
There was a problem hiding this comment.
don't use logger with String.format, logger has internal support for placeholders and exception.
see this:
http://www.slf4j.org/faq.html#exception_message
| logger.error(String.format("Cosmos getStarted failed with %s", e)); | |
| logger.error("Cosmos getStarget failed", e) |
|
|
||
| private void containerCRUDDemo() throws Exception { | ||
|
|
||
| logger.info("Using Azure Cosmos DB endpoint: " + AccountSettings.HOST); |
There was a problem hiding this comment.
rely on slf4j placeholder rather than string concatination
see this:
http://www.slf4j.org/faq.html#paramException
| logger.info("Using Azure Cosmos DB endpoint: " + AccountSettings.HOST); | |
| logger.info("Using Azure Cosmos DB endpoint: {}", AccountSettings.HOST); |
| readContainerById(); | ||
| readAllContainers(); | ||
| // deleteAContainer() is called at shutdown() | ||
|
|
There was a problem hiding this comment.
remove the newline.
|
|
||
| // Update container throughput | ||
| private void updateContainerThroughput() throws Exception { | ||
| logger.info("Update throughput for container " + containerName + "."); |
| containerProperties.setAnalyticalStoreTimeToLiveInSeconds(-1); | ||
|
|
||
| // Create container with 200 RU/s | ||
| container = database.createContainerIfNotExists(containerProperties, 200).getContainer(); |
There was a problem hiding this comment.
there is some discussion about removing this API which accept throughput as int in favour of the new offer apis, sync with Bhaskar on this.
| deleteADatabase(); | ||
| } catch (Exception err) { | ||
| logger.error("Deleting Cosmos DB resources failed, will still attempt to close the client. See stack trace below."); | ||
| err.printStackTrace(); |
There was a problem hiding this comment.
remove err.printStackTrace, use logger for exception logging
logger.error("Deleting Cosmos DB resources failed, will still attempt to close the client.", err);| 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)); |
| private Child[] children; | ||
| private Address address; | ||
| private boolean isRegistered; | ||
| private String id=""; |
There was a problem hiding this comment.
code style:
please try to format the changes, spacing, etc
Intellij has support for code style formatting
https://moduscreate.com/blog/12-intellij-idea-keyboard-shortcuts/
|
@RyanFeldman please keep track of the comments here. I am closing this PR as I had created this PR for a place holder so I can add the comments. |
[11:51 AM] Andy Feldman
Hi Mo. Yes I was forcing parallelism of 1 for that test. So I went ahead and kept the result I mentioned (smile)
[11:51 AM] Andy Feldman
Migration guide:
https://github.com/Azure-Samples/azure-cosmos-java-sql-api-samples/blob/master/migration-guide.md
Reactor vs RxJava:
https://github.com/Azure-Samples/azure-cosmos-java-sql-api-samples/blob/master/reactor-rxjava-guide.md
Reactor Pattern Guide:
https://github.com/Azure-Samples/azure-cosmos-java-sql-api-samples/blob/master/reactor-pattern-guide.md
Azure-Samples/azure-cosmos-java-sql-api-samplesSample code for Azure Cosmos DB Java SDK for SQL API - Azure-Samples/azure-cosmos-java-sql-api-samplesgithub.com[11:51 AM] Andy Feldman
When you have some time, I would greatly appreciate your feedback on these three docs
[11:52 AM] Andy Feldman
In terms of correctness, what is a must-add, and what could be removed.