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.

Sign up for GitHub

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

Fixes based on feedback #51

Closed
wants to merge 6 commits into from
Closed

Fixes based on feedback #51

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6df21af
Fixed readme based on feedback for system service
evelinec Mar 20, 2018
2cd66a9
Minor changes
evelinec Mar 20, 2018
f41a0f7
Fixed thread saft issue
evelinec Mar 23, 2018
341340a
Fixed readme with feedback and reworked quoted statements
evelinec Mar 28, 2018
fd63596
Minor change in readme
evelinec Apr 13, 2018
7c96a66
Merge branch 'master' into newEdits
evelinec Jun 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ They dictate whether and when executions take place, and fallbacks offer an alte
when an execution does not complete successfully.
Copy link
Member

Choose a reason for hiding this comment

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

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


The application that you will be working with is an `inventory` service, which collects,
stores, and returns the system properties.
stores, and returns system properties.
It uses the `system` service to retrieve the system properties for a particular host.
You will add fault tolerance to the `inventory` service so that it reacts accordingly when the `system`
service is unavailable.
Expand Down Expand Up @@ -69,16 +69,15 @@ mvn clean install liberty:start-server
```

Point your browser to the `http://localhost:9080/inventory/systems/localhost` URL, which accesses the
`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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

When you visit this URL, only the OS name and user name properties are stored in the inventory.

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


When you are done checking out the application, go to the `CustomConfigSource.json` file again and
change the `io_openliberty_guides_system_inMaintenance` property from `true` to `false` to set this
Expand Down Expand Up @@ -106,7 +105,7 @@ Adding this dependency allows you to use the fault tolerance policies in your mi
You can also find the `mpFaultTolerance-1.0` feature in your `src/main/liberty/config/server.xml` server configuration,
which turns on MicroProfile Fault Tolerance capabilities in Open Liberty.

Now, you need a failure to occur in order for fault tolerance fallback mechanism to take place and ensure the continuity of the microservice.
You need a failure to occur in order for fault tolerance fallback mechanism to take place and ensure the continuity of the microservice.
In this guide, the failure is to throw an exception when the `system` service is not available, or down for maintenance.
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.
Expand All @@ -130,7 +129,7 @@ the service client class in the `src/main/java/io/openliberty/guides/inventory/c

The `inventory` service is now able to recognize that the `system` service
was taken down for maintenance.
An IOException is thrown to simulate the `system` service is unavailable.
An IOException is thrown to simulate that the `system` service is unavailable.
Now, set a fallback method to deal with this failure.

Create the `InventoryManager` class in the
Expand Down Expand Up @@ -160,7 +159,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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

When you visit this URL, only the OS name and user name properties are stored in the inventory. Next, point your
browser to the `system` service URL, which is located at `http://localhost:9080/system/properties`,
to retrieve the system properties for the specific localhost.
Notice that the results from the two URLs are identical because the `inventory` service gets its results from
Expand All @@ -171,9 +171,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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

The `fallbackForGet()` method, which is the designated fallback method, was called
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

when the `system` service is not available.
The cached system properties contain only the OS name and user name key and value pairs.

To see that the `system` service is down, point your browser to the `http://localhost:9080/system/properties` URL again.
You see that the service displays a 503 HTTP response code.
Expand Down Expand Up @@ -212,6 +212,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.
Copy link
Member

Choose a reason for hiding this comment

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

'connects'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


In addition, a few endpoint tests have been included for you to test the basic functionality of the
`inventory` and `system` services. If a test failure occurs, then you might have introduced a bug into
the code.
Expand Down Expand Up @@ -239,9 +241,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
Copy link
Contributor

Choose a reason for hiding this comment

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

Would "delete" suffice for "comment out"?

in the `src/main/java/io/openliberty/guides/inventory/InventoryManager.java` file. Rerun the Maven build.
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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor

@sishida sishida Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.



// =================================================================================================
Expand Down
6 changes: 3 additions & 3 deletions finish/src/test/java/it/io/openliberty/guides/faulttolerance/FaultToleranceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,19 @@ public void teardown() {
public void testFallbackForGet() throws InterruptedException {
response = TestUtils.getResponse(client,
TestUtils.INVENTORY_LOCALHOST_URL);
assertResponse(TestUtils.baseUrl, response);
assertResponse(TestUtils.BASE_URL, response);
JsonObject obj = response.readEntity(JsonObject.class);
int propertiesSize = obj.size();
TestUtils.changeSystemProperty(TestUtils.SYSTEM_MAINTENANCE_FALSE,
TestUtils.SYSTEM_MAINTENANCE_TRUE);
Thread.sleep(3000);
response = TestUtils.getResponse(client,
TestUtils.INVENTORY_LOCALHOST_URL);
assertResponse(TestUtils.baseUrl, response);
assertResponse(TestUtils.BASE_URL, response);
obj = response.readEntity(JsonObject.class);
int propertiesSizeFallBack = obj.size();
assertTrue("The total number of properties from the @Fallback method "
+ "is not smaller than the number from the system service, as expected.",
+ "should be smaller than the number from the system service.",
propertiesSize > propertiesSizeFallBack);
TestUtils.changeSystemProperty(TestUtils.SYSTEM_MAINTENANCE_TRUE,
TestUtils.SYSTEM_MAINTENANCE_FALSE);
Expand Down
4 changes: 2 additions & 2 deletions finish/src/test/java/it/io/openliberty/guides/utils/TestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ public class TestUtils {

private static String port = System.getProperty("liberty.test.port");

public static String baseUrl = "http://localhost:" + port + "/";
public static final String INVENTORY_LOCALHOST_URL = baseUrl + "inventory/systems/localhost/";
public static String BASE_URL = "http://localhost:" + port + "/";
public static final String INVENTORY_LOCALHOST_URL = BASE_URL + "inventory/systems/localhost/";
public static final String SYSTEM_MAINTENANCE_FALSE = "io_openliberty_guides_system_inMaintenance\":false";
public static final String SYSTEM_MAINTENANCE_TRUE = "io_openliberty_guides_system_inMaintenance\":true";

Expand Down