Fixes based on feedback #51

evelinec · 2018-03-20T18:26:41Z

Fixed readme based on feedback for system service

evelinec · 2018-03-20T18:29:40Z

@yeekangc @JunyiSun pls review the re-written paragraph for the system service and client classes.
Thanks.

JunyiSun · 2018-03-20T19:10:31Z

@evelinec I think there Grace added the 503 exception there which is another kind of failure.

evelinec · 2018-03-22T14:59:58Z

Sorted out regarding @JunyiSun review comment above, that there's no other kind of failure.

yeekangc

Provided comments previously that should be addressed.

yeekangc · 2018-03-20T20:38:16Z

README.adoc

+The `system` client that handles the service being in maintenance and then throws an exception is also provided for you.
+If you want to see the simulation and setup,
+refer to the `system` service class in the `src/main/java/io/openliberty/guides/system/SystemResource.java` file, and
+the service client class in the `src/main/java/io/openliberty/guides/inventory/client/SystemClient.java` file.


This section here seems unnecessary. I find the use of "simulation" here weird too.

It is good to have a pointer to the MP Config guide when the config property is mentioned. I am inclined to take away the others.

yeekangc · 2018-03-20T20:45:38Z

README.adoc

+First, dynamic configuration from MicroProfile Configuration is used to easily manage the availability of the `system` service.
+A configuration is injected into the `system` service to dynamically read a system property that you can set to bring the service up or down.
+With this configuration set in place, the next step is to have the service client to react with a failure when the service is down.
+The failure is to throw an IOException.


To demonstrate the @Fallback mechanism, we simulate that the system service is unavailable.

The system service is injected with a dynamic configuration property using MicroProfile Configuration that enables you to take it down or make it up easily. By changing this property, the system service can be taken down quickly, simulating a failure resulting in an exception that triggers the @Fallback mechanism.

Don't think we'd use "we". Will rework this one.

Sure. Provided the above for illustration only.

yeekangc · 2018-03-20T20:46:47Z

README.adoc


 Now, open the `resources/CustomConfigSource.json` file and change the
 `io_openliberty_guides_system_inMaintenance` property from `false` to `true` and save the file. You do not need
 to restart the server. Next, return to your browser and point back to the
 `http://localhost:9080/inventory/systems/localhost` URL.
 The fallback mechanism is triggered because the `system` service is now in maintenance.
-You see the cached properties for this localhost.
+You see the cached properties that were stored in the inventory.


Add 'previously' to before 'stored' to make it clearer?

evelinec · 2018-03-28T18:10:32Z

@yeekangc when you have a chance, can you review the latest changes. Thanks.

Azquelt · 2018-04-01T10:49:42Z

The InventoryList changes look good 👍

yeekangc · 2018-03-23T17:03:39Z

README.adoc

@@ -36,7 +36,7 @@ They dictate whether and when executions take place, and fallbacks offer an alte
 when an execution does not complete successfully.


The whole paragraph above should be rewritten. We should write it in our own words.

yeekangc · 2018-04-12T20:00:19Z

README.adoc

-set the `io_openliberty_guides_system_inMaintenance` configuration property
-to `false` in the `resources/CustomConfigSource.json` file.
-Otherwise, the service returns a `Status.SERVICE_UNAVAILABLE` message, which makes it unavailable.
+To demonstrate the `@Fallback` mechanism, the microservice simulates the `system` service is in maintenance.


Please consider if we should expand here. Or, at least, revise the sentence ... What is "the microservice"? Are we skipping too much?

The "microservice" is described in the intro section. It may be good to see the merged version and if additional expansion is needed.

This "the microservice" shows up for the first time here in this section. It still begs the question what is this "the microservice". IMO, we should revise and clarify what we are trying to convey here.

yeekangc · 2018-04-12T20:01:52Z

README.adoc

-You will look into the `InventoryManager` class in more detail in the next section.
+The configuration for the `system` service being in maintenance is provided for you.
+To learn more about dynamic configuration and how it’s enabled in a microservice, see
+https://openliberty.io/guides/microprofile-config.html[Configuring microservices].


Is this not a repeat of the paragraph above? Not sure what's the distinction?

(It will be easier to review everything in context.)

Yes, it'll be easier to review the everything in context. These two sentences are to point to the "Configuring microservices" guide. Don't think it's being done everywhere else.

I would suggest that we consider reworking the 2 paragraphs together.

yeekangc · 2018-04-12T20:03:41Z

README.adoc

@@ -173,7 +149,8 @@ include::{common-includes}/mvnbuild.adoc[]

 When the server is running, point your browser to the
 `http://localhost:9080/inventory/systems/localhost` URL.
-You receive the system properties of your local JVM from the `inventory` service. Next, point your
+You receive all of the system properties in your local JVM.


I don't think typically the term, receive, is used in such cases?

The distinction between the inventory service and system service is unclear still in this paragraph or section?

Fixed the "receive".

The distinction between the inventory and system services had been explained through out the guide, ie, in the intro, and in the "Try what you'll build" section with the URL used there. Not sure we want repeat it again in the paragraph/section.

yeekangc · 2018-04-12T20:04:15Z

README.adoc

@@ -225,6 +202,8 @@ to ensure that they are different from each other.

 The `@Test` annotation indicates that the method automatically executes when your test class runs.

+The TestUtils class includes utility methods to make connects to the services and automate property changes.


'connects'?

yeekangc · 2018-04-12T20:05:18Z

@evelinec, please consider if there should be ID review when ready.

yeekangc · 2018-04-12T20:06:19Z

README.adoc

 You see a test failure occur for the `testFallbackForGet()` test case.
+The test failure indicates a 500 HTTP response code because the fallback mechanism was not in place to action
+when the IOException was thrown.


We have discussed that we need more to address #44. Just noting it.

yeekangc · 2018-04-12T20:09:10Z

finish/src/main/java/io/openliberty/guides/inventory/model/InventoryList.java


 public class InventoryList {

-    private List<SystemEntry> systems = new ArrayList<SystemEntry>();
-
+    private List<SystemEntry> systems = new CopyOnWriteArrayList<SystemEntry>();


Do we need this?

This is needed to fix the thread safe issue. There was another approach, which used the ArrayList, but then adding a lock for the get method. This will make the get method less efficient. Our application calls the get methods more often than others, so this approach was chosen. I presented both to Andrew, and he thinks this one is good.

evelinec · 2018-04-13T14:51:07Z

@yeekangc Regarding having ID review, yes, we still need ID review for these changes. Sorry, it's been so long, it's been forgotten. After the latest feedback are good, will request ID review.

evelinec · 2018-04-13T18:42:56Z

@sishida can you review the changes in the readme for this guide? Thank you.

sishida · 2018-04-17T13:25:11Z

README.adoc

-service is unavailable.
-
-You will use the `@Fallback` annotations from the MicroProfile Fault Tolerance
+You will build the fault-tolerance microserivce with the


Fix typo: "microserivce" should be "microservice."

sishida · 2018-04-17T13:27:51Z

README.adoc

+The microservice that you will be working with is an `inventory` service, which collects,
+stores, and returns system properties.
+It uses the `system` service to retrieve the system properties for a particular host.
+You will add fallback mechanism to the `inventory` service so that it reacts accordingly when the `system`


Change to "You will add the fallback..."

sishida · 2018-04-17T13:30:22Z

README.adoc

-
-You will use the `@Fallback` annotations from the MicroProfile Fault Tolerance
+You will build the fault-tolerance microserivce with the
+`@Fallback` annotation from the MP Fault Tolerance


I'm not sure if "MP" is familiar enough that users will automatically know what it stands for. While we don't need to spell out abbreviations like HTTP or URL, I'd say "MicroProfile (MP) Fault Tolerance" here. :)

The "MicroProfile (MP)" was introduced at the very beginning of the guide, thus it looks ok to use the abbreviation here.

sishida · 2018-04-17T13:40:03Z

README.adoc

-`inventory` service with a localhost host name. You see the system properties for this host.
-When you visit this URL, some of these system
-properties, such as the OS name and user name, are automatically stored in the inventory.
+`inventory` service with a localhost host name. You see all of the system properties for this host from the `system` service.


I think that we should format localhost as a code phrase, localhost.

We should probably format it like this across the whole doc.

There are 2 occurrences of localhost being referred to as host name. Those are fixed with back ticks. Thanks.

sishida · 2018-04-17T13:47:07Z

README.adoc

-Otherwise, the service returns a `Status.SERVICE_UNAVAILABLE` message, which makes it unavailable.
+To demonstrate the `@Fallback` mechanism, the microservice simulates the `system` service is in maintenance.
+The `system` service is injected with a dynamic configuration property by using MicroProfile Configuration.
+The configuration enables you to take it down or bring it up easily. By changing this property,


Try to avoid directional words like "up" and "down." (Maybe "turn it off" or "turn it on" instead?) However, if "up" and "down" work better, feel free to leave it as it is. This comment also applies to line 106.

I would also specify what "it" is, such as replacing the word "it" with "the service" at least in the first instance of the word "it" here.

sishida · 2018-04-17T13:48:03Z

README.adoc

+To demonstrate the `@Fallback` mechanism, the microservice simulates the `system` service is in maintenance.
+The `system` service is injected with a dynamic configuration property by using MicroProfile Configuration.
+The configuration enables you to take it down or bring it up easily. By changing this property,
+the `system` service can be taken down quickly, simulating a failure that results in an exception,


Could we remove the passive voice in this sentence by saying instead, "By changing this property, you can quickly take down the system service, simulating..."?

sishida · 2018-04-17T13:51:35Z

README.adoc

+The `system` service is injected with a dynamic configuration property by using MicroProfile Configuration.
+The configuration enables you to take it down or bring it up easily. By changing this property,
+the `system` service can be taken down quickly, simulating a failure that results in an exception,
+which triggers the `@Fallback` mechanism.


We've strung a few phrases together here. I'd suggest making the last phrase its own sentence: "...simulating a failure that results in an exception. The exception then triggers the..."

sishida · 2018-04-17T13:52:26Z

README.adoc

-The public `getProperties()` method in this class calls the `getPropertiesHelper()` method.
-The `InventoryManager` class calls this public method.
-You will look into the `InventoryManager` class in more detail in the next section.
+The configuration for the `system` service being in maintenance is provided for you.


I recommend "...service that is in maintenance..."

sishida · 2018-04-17T13:56:27Z

README.adoc

@@ -184,9 +161,9 @@ the `io_openliberty_guides_system_inMaintenance` property value to `true` in the
 `resources/CustomConfigSource.json` file, which turns the `system` service in maintenance.
 After saving the file, go back to your browser and
 refresh to the `http://localhost:9080/inventory/systems/localhost` URL to view the cached version of
-the properties. The `fallbackForGet()` method, which is the designated fallback method, is called
+the properties, which contain only the OS name and user name key and value pairs.


Use a serial comma here: "...contain only the OS name key, user name key, and value pairs." or "...contain only the OS name, user name key, and value pairs."
As the sentence is, I am not sure if "key" refers to both the OS name and the user name or just the user name. The list as shown above can help clarify.

sishida · 2018-04-17T13:58:38Z

README.adoc

@@ -184,9 +161,9 @@ the `io_openliberty_guides_system_inMaintenance` property value to `true` in the
 `resources/CustomConfigSource.json` file, which turns the `system` service in maintenance.
 After saving the file, go back to your browser and
 refresh to the `http://localhost:9080/inventory/systems/localhost` URL to view the cached version of
-the properties. The `fallbackForGet()` method, which is the designated fallback method, is called
+the properties, which contain only the OS name and user name key and value pairs.
+The `fallbackForGet()` method, which is the designated fallback method, was called


Say "is called" instead of "was called."

sishida · 2018-04-17T14:00:39Z

README.adoc

@@ -225,6 +202,8 @@ to ensure that they are different from each other.

 The `@Test` annotation indicates that the method automatically executes when your test class runs.

+The TestUtils class includes utility methods to make connection to the services and automate property changes.


It looks like TestUtils should be formatted as a code phrase.

Also what if we added the two words in bold below?
"The TestUtils class includes utility methods to make a connection to the services and to automate property changes."

sishida · 2018-04-17T14:01:13Z

README.adoc

@@ -252,9 +231,11 @@ Results :
 Tests run: 3, Failures: 0, Errors: 0, Skipped: 0
 ----

-To see if the tests detect a failure, comment out the `changeSystemProperty()` methods
-in the `FaultToleranceTest.java` file. Rerun the Maven build.
+To see if the tests detect a failure, comment out the `@Fallback` annotation


Would "delete" suffice for "comment out"?

sishida · 2018-04-17T14:07:27Z

README.adoc

 You see a test failure occur for the `testFallbackForGet()` test case.
+The test failure indicates a 500 HTTP response code because the fallback mechanism was not in place to action
+when the IOException was thrown.


We can make this sentence present tense. Can we also find a more common word for "to action," such as "to run" or "to start"? (I usually see "action" as a noun.)

"The test failure indicates a 500 HTTP response code because the fallback mechanism is not in place to run when the IOException is thrown."

Do we also need to format "500 HTTP" response as code? I'll leave that up to you. :)

Note from my grammar checker about the word "thrown": "Use only in documentation of programming languages (such as Java, C++, Ruby, Lisp or others) that support throw and catch programming blocks, as in "throw an exception." Otherwise, use an alternative construction such as "produce an error.""
I think it's OK here, but, once again, I'll let you decide.

sishida · 2018-04-17T14:13:44Z

I have reviewed all the doc changes and added comments. Thanks!

JunyiSun

@evelinec seems like Sarah's suggestions haven't been incorporated yet.

evelinec · 2020-01-15T17:25:31Z

Re-reviewed the changes made here with the comparison of the latest version of the guide, most of these are not needed anymore. The relevant feedback and changes will be (and better to be) addressed in new PRs instead, since this one is quite outdated. Closed.

Fixed readme based on feedback for system service

6df21af

evelinec requested review from yeekangc and JunyiSun March 20, 2018 18:27

Minor changes

2cd66a9

yeekangc requested changes Mar 22, 2018

View reviewed changes

evelinec added 2 commits March 23, 2018 10:54

Fixed thread saft issue

f41a0f7

Fixed readme with feedback and reworked quoted statements

341340a

This was referenced Mar 28, 2018

InventoryList is not thread safe #49

Closed

FT: Last test review #48

Closed

FT: Review feedback #46

Closed

Better exercise for users to experiment with automated tests #44

Closed

evelinec requested review from sishida and removed request for sishida March 28, 2018 04:50

yeekangc requested changes Apr 12, 2018

View reviewed changes

Minor change in readme

fd63596

evelinec requested a review from sishida April 13, 2018 18:42

sishida reviewed Apr 17, 2018

View reviewed changes

sishida self-assigned this Apr 17, 2018

JunyiSun reviewed May 25, 2018

View reviewed changes

Merge branch 'master' into newEdits

7c96a66

evelinec closed this Jan 15, 2020

gkwan-ibm deleted the newEdits branch May 29, 2020 20:40

		@@ -36,7 +36,7 @@ They dictate whether and when executions take place, and fallbacks offer an alte
		when an execution does not complete successfully.

		@@ -225,6 +202,8 @@ to ensure that they are different from each other.

		The `@Test` annotation indicates that the method automatically executes when your test class runs.

		The TestUtils class includes utility methods to make connects to the services and automate property changes.

Fixes based on feedback #51

Fixes based on feedback #51

Conversation

evelinec commented Mar 20, 2018

evelinec commented Mar 20, 2018 • edited Loading

JunyiSun commented Mar 20, 2018

evelinec commented Mar 22, 2018

yeekangc 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

evelinec commented Mar 28, 2018

Azquelt commented Apr 1, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeekangc commented Apr 12, 2018

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

evelinec commented Apr 13, 2018 • edited Loading

evelinec commented Apr 13, 2018

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

Choose a reason for hiding this comment

sishida Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sishida Apr 17, 2018 • edited Loading

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

sishida Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sishida commented Apr 17, 2018

JunyiSun left a comment

Choose a reason for hiding this comment

evelinec commented Jan 15, 2020

evelinec commented Mar 20, 2018 •

edited

Loading

evelinec commented Apr 13, 2018 •

edited

Loading

sishida Apr 17, 2018 •

edited

Loading

sishida Apr 17, 2018 •

edited

Loading

sishida Apr 17, 2018 •

edited

Loading