openliberty / guide-microprofile-istio-retry-fallback Goto Github PK

Peer Review: review to be done by a peer member.

Functionality

• Check that the guide has working instructions and sample code
• Test the guide on 3 platforms: Mac, Win, Linux
• If any URL visits, try them on different browsers: Firefox, Chrome, Safari
• If any URL visits, try curl command where applicable

Formatting & Presentation

• Check the quality of code according to the best coding practices
  • Ensure that command substitutions use the $(cmd) format and not `cmd`
  • Ensure that the console output of the integrated tests do not include the [INFO] tab
• Check the quality and presentation of guide according to Structure and Style Guideline
• Check the consistency of guide with template and other guides
• Examples with the right outcomes are provided
• Example with the wrong outcomes, if any, are provided
• Check that all licensing statements are properly stated in all files
• Check that the pom.xml, server.xml, etc files are clean
• Check that the directories are properly structured
• Ensure that the guide is using the latest version of the liberty-maven-plugin or liberty-gradle-plugin where applicable
• Check that some of these page-tags are used in a guide: MicroProfile, Maven, Docker, Kubernetes, Gradle, Java EE, Security, Cloud. Only these tags are visible on the website. Latest list here.
• Check the attribution statement is accurate for the guide
• Verify the travisTest.sh script, if any, is accurate and consistent with other guides

Peer Testing: tests to be done by a peer member.

Guide’s contributor’s (if available, otherwise peer tester’s) responsibility:

• Additional tests where applicable:
  • Define test coverage and review with team (including guide contributor, if available)
  • Define detail test cases
  • Consider corner cases targeting the specific guide
  • Consider different platform tests, ie, Mac, Windows, Linux
  • Consider corner cases UI tests
  • Consider testing URL on all browsers, ie, FF, Chrome, Safari
  • Consider testing the curl command for URL visits
• Consider building with both Maven and Gradle build tools
• Testing with different IDEs, ie, Atom, Eclipse (Optional: VS.code, IntelliJ, Microclimate)
• Run Acrolinx Checker on draft (above 70 score approximately)
• Consider SEO title and description for the guides
• Ensure automated test is enable, set up with Travis CI, and able to schedule tests
• Run diff -r start/ finish/ and there's no differences
• Ensure that the automation tests are able to run when PR is created

Peer Tester’s responsibility:

• Check the appearance of the guide on test site for the following items:
  • Table of contents
  • Headings
  • Paragraphs
  • code snippets
  • outputs
  • links
  • hotspots
• Test the guide end-to-end with working instruction and sample code
• Perform all the defined test cases

Guide review- developing FT MS w: IS 7 MP.docx

Update the guide to use istio 1.20.0

• update README output
• update test case

• "Fault tolerance leverages different strategies to guide the execution and result of logic": What do we mean by "result of logic"?

• "Use the Retry policy to fail quickly and recover from brief intermittent issues. An application might experience these transient failures when a microservice is undeployed, a database is overloaded by queries, the network connection becomes unstable, or the site host has a brief downtime. In these cases, rather than failing quickly on these transient failures, the Retry policy provides another chance for the request to succeed. Simply retrying the request might be all you need to do to make it succeed": What is "the Retry policy" here? Are we talking about generic retry capabilities or we are referring to something specific from MicroProfile or Istio?

• "Fallback offers an alternative result when an execution does not complete successfully": "Alternative result"? It should be more an alternative execution path or option?

• "You will create a microservice ecosystem demonstrating MicroProfile Fault Tolerance with Istio fault handling": What do we mean by "microservice ecosystem"? I will use a simpler term.

• "Both libraries can be enabled when you want your microservices to have a service mesh architecture with Istio, and use MicroProfile to provide the extra fault tolerance policies that do not exist within Istio": Are they actually "libraries" that can be enabled?

• "However, microservices that have the same fault tolerance policy enabled by both MicroProfile and Istio will have a behaviour that is not as expected": What does it mean by "behaviour that is not as expected"? This seems to be contrary to what the guide is trying to convey i.e. MicroProfile and Istio can work together?

• "The Host header of your system service and inventory service to be system.example.com and inventory.example.com, respectively": Missing a verb or something with this sentence?

• "Now you will make the system service unavailable and MicroProfile’s Retry policy will be performed": Consider something like "... and observe that MicroProfile's Retry policy will take effect"?

• "However, the request was retried several times with MicroProfile @Retry before failing": Consider "... retried several times as per what is specified by MicroProfile @Retry annotation ..."?

• "After integrating an Istio Retry policy into your microservices, deploy your microservices again": We aren't integrating Istio Retry policy into the microservices here? We are specifying what we need Istio to do?

• "Later, you will disable some of MicroProfile’s capabilities, to have your system service retry with only Istio’s Retry policy": Not sure if we really need this sentence. If we do keep it, I will change "Later" to "Next" and clarify that that they are "MicroProfile Fault Tolerance capabilities"?

• "When microservices have the same fault tolerance policy enabled by both MicroProfile and Istio, the behaviour of the policy is not as expected": I will rephrase this. Perhaps consider something like "When we have both MicroProfile and Istio fault tolerance capabilities enabled, there is a compounding effect that may be unexpected".

• "The microservice actually multiplies the number of MicroProfile and Istio retries": It's not the microservice that is multiplying the retries. This sentence needs to be rewritten.

• "In the case that you want your microservices to use a service-mesh architecture, such as Istio, and use it’s fault handling without interfering with the service’s MicroProfile capabilities, you can use a config property offered by MicroProfile.": "In the case that you want to use Istio as your service mesh and its fault tolerance capabilities, you can turn off MicroProfile Fault Tolerance via a property to avoid any overlap in behaviours."

• "MP_Fault_Tolerance_NonFallback_Enabled is set to false, all of the MicroProfile annotations, including @Retry, will be switched off and only keep @fallback available to use": It's not so much that the annotations are switched off. They are still there but the specified behaviours won't take effect since the runtime will follow what is specified by the property i.e. turn those off.

• "The MP_Fault_Tolerance_NonFallback_Enabled config property must be defined in a ConfigMap": It is not a must to define this in a ConfigMap. Then, since we are operating in a Kube environment, doing this is the natural way. See the guide on Kube and MicroProfile Config.

• "Deploy your microservices again to turn off all MicroProfile capabilities, except fallback": We need to be careful. It isn't "all MicroProfile capabilities" but the Fault Tolerance ones minus fallback.

• "Since retrying the requests to your system service is still not successful, you need a plan to "fall back" on when the requests fail. You will create a fallback method to deal with the failed requests to your system service": Consider something like "Since retrying the requests to the system service still does not give you success, you need a "fall back" plan. You will create a fallback method as an alternative solution when the strategy to retry requests to the system service failed".

• "As mentioned before, Istio does not offer any fallback capabilities, and so the MicroProfile @fallback annotation must be used to add fallback to your microservices": Considering something like "... Istio does not offer any fallback capabilities, so MicroProfile Fallback capability can be used to complement it".

• "Rebuild your application and integrate the Fallback policy into your microservices": I find the term "integrate" odd. Consider an alternate term or rephrase.

• "Deploy your microservices again to turn off all MicroProfile capabilities, except fallback": It is not "all MicroProfile capabilities".

• Do we need to talk about "x-from-fallback: yes"?

@gkwan-ibm @NimG98

Post-Publishing Tests

• Check appearance (contents, headings, paragraphs, code snippets, outputs, links) of the guide on openliberty.io
• Clone repo and test finish directory with no error

Other post-publishing tasks (to be done by the Admin Team)

• Add description for the repo with link to guide's published website
• Double check the SEO title and description are added for the guide
• Create the Home and Branches wiki pages
• Ensure automated test for PR and Continuous Tests are enabled for both master and qa branches and are run on Travis, see instruction
• Ensure the automated tests are running successfully for the daily builds
• Ensure the automated tests are running successfully for the daily docker image, if the guide uses a docker image
• Enable pull creation notification on the repo for was-lib-guides-alerts, see instruction
• Enable master and qa branches lock down, see instruction
  • make sure the Branch protection rules were created
• Add the new guide's repo to the OpenLiberty/guides-common ZenHub board
• Delete dev branch
• (Optional) Send announcement to slack channel(s)
• Arrange a demo at the EOI meeting

Publishing Tasks

• Create a PR from dev to master branch
• Create qa branch from master branch
• Rename the guide repo
• Refresh qa staging site and verify
• If qa site has no problem, request to refresh openliberty.io site
• For master branch, add a tag by creating a new release for this repo with name "1st-release"

Use microprofile-istio-retry-fallback as the ID and hence, guide-microprofile-istio-retry-fallback as the slug and repo name. I believe this better conveys what we intend to show with this guide and is better for SEO too.

FYI, @gkwan-ibm.

Checklist

• Record the time it takes to go through the entire guide
• Test each step in the guide is clear and easy to understand
• Ensure no logic errors
• Report anything that does not work
• Report anything usability issues (confusing or hard to follow)
• Achieve basic understanding/working knowledge of the topic in the guide

Enabling MicroProfile Fault Tolerance

The location of server.xml is not apparent if you are not already familiar with the file architecture. The full path should be displayed so that it is easier to find

The justification for 37 being the number of retries of the service is a bit unclear. For a novice it is a bit confusing.

Overall

Guide is easy to follow and understand.

Time taken

~30 Minutes

1. In server.xml, why do you need mpMetrics feature?
2. since you are using mpConfig,
   private final String SYS_HTTP_PORT = System.getProperty("system.http.port");
   should use MP Config to inject.
3. Why don't you inject the rest client via @Inject @RestClient

• Activate automated test for PR for dev branch to run on Travis

Peer Review: review to be done by a peer member.

• Check that the guide has working instruction and sample code
• Check the quality of code according to the best coding practices
• Check the quality and presentation of guide according to Structure and Style Guideline
• Check the consistency of guide with template and other guides
• Examples with the right outcomes are provided
• Example with the wrong outcomes, if any, are provided
• Check that all licensing statements are properly stated in all files
• Check that the pom.xml, server.xml, etc files are clean
• Check that the directories are properly structured
• Test the guide on 3 platforms: Mac, Win, Linux (Tested on Mac, @DannyS03 is responsible for the other OS tests)
• If any URL visits, try them on different browsers: Firefox, Chrome, Safari (Used the curl command)
• If any URL visits, try curl command where applicable
• Ensure that the guide is using the latest version of the liberty-maven-plugin or liberty-gradle-plugin where applicable
• Check that some of these page-tags are used in a guide: MicroProfile, Maven, Docker, Kubernetes, Gradle, Java EE, Security, Cloud. Only these tags are visible on the website. Latest list here.
• Check the attribution statement is accurate for the guide
• Verify the travisTest.sh script, if any, is accurate and consistent with other guides

Peer Testing: tests to be done by a peer member.

Peer Tester’s responsibility:

• Check the appearance of the guide on test site for the following items:
• Table of contents
• Headings
• Paragraphs
• code snippets
• outputs
• links
• hotspots
• Test the guide end-to-end with working instruction and sample code
• Perform all the defined test cases

Test ran on MacOS, complete end-to-end.

• include MicroProfile in the page-tags

• licence block missing from start/../InventoryResource.java, double check all other .java files to ensure they have the license blocks.

Deploying Istio

• Next, deploy Istio resources to your cluster by running the kubectl apply command, which creates or updates Kubernetes resources defined in a yaml file. This command deploys Istio.
  "This command deploys istio is redundant since the start of the sentence states that you're deploying istio resources.

• Verify that Istio was successfully deployed. All the values in the AVAILABLE column will have a value of 1 after the deployment is complete. Should change to Verify that Istio was successfully deployed by running the following command: and move the All the values in the AVAILABLE column will have a value of 1 after the deployment is complete. to after the command block for better flow.

• Output of the kubectl get deployments -n istio-system command is different, see my output below:

NAME                     READY   UP-TO-DATE   AVAILABLE   AGE
grafana                  0/1     1            0           6s
istio-citadel            0/1     1            0           6s
istio-egressgateway      0/1     1            0           6s
istio-galley             0/1     1            0           6s
istio-ingressgateway     0/1     1            0           6s
istio-pilot              0/1     1            0           6s
istio-policy             0/1     1            0           6s
istio-sidecar-injector   0/1     1            0           6s
istio-telemetry          0/1     1            0           6s
istio-tracing            0/1     1            0           5s
kiali                    0/1     1            0           6s
prometheus               0/1     1            0           6s

• The line You don’t need to perform any additional steps for the sidecars to be injected. seems unnecessary, and bloats the paragraph it is in.

Adding the MicroProfile @Retry annotation

• When telling the user to add the @Retry annotation, the @fallback annotation also shows in the code pane, which doesn't exist in the InventoryResource.java file yet since the guide tells the user to add it in the later section. I suggest hiding this property in this section as well as any other related Fallback lines of code.
  

• Since the default number of retries for the maxRetries flag is 3, its redundant to include the flag just to set it to the default value. I understand it is for demonstration purposes but either use a different value or remove it entirely.

Building and running the application

• The above command returns 1, meaning that 1 request is made to the system service. Your single request to the service is successful with a 200 response code. This sentence is unnecessarily long and repeats the same thing, I suggest making it more succinct, at the very least removing The above command returns 1

• Guide flow inconsistency, I recommend swapping the Adding the MicroProfile @Retry section with the Building and running the application section. Why do we talk about and implement the @Retry annotation if the next section shows the application running without issues? It would make sense to showcase the working application and then explaining the situation of the application failing, THEN introducing @Retry

• When using curl on the paused system pod, my error is HTTP/1.1 503 Service Unavailable instead of the guide HTTP/1.1 503 Internal Server Error

• Maybe further explain why the number of retries is 37, since the @Retry annotation was set to the default number of retries of 3, so is the remaining 34 by MicroProfile?

Enabling Istio fault tolerance

• Grammar inconsistency, if you capitalize each first letter in MicroProfile Fault Tolerance, the same should be applied to Istio Fault Tolerance, it is uncapitalized in multiple locations.

• Specify that the retries field is located under the route spec in the updating of the traffic.yaml file.

• The hotspot includes an extra unnecessary blank line.
  

• The above command returns 60, indicating that a total of 60 requests are made to the system service. These requests include Istio’s default number of retries, the retries that you enabled in the traffic.yaml file, and the retries set by MicroProfile. Same as comment above re: 1.

• You will disable some of MicroProfile’s capabilities, including Retry, to have your system service retry with only Istio’s Retry policy. Reword, its awkward. Maybe try Next, you will... off the top of my head?

Turning off MicroProfile Fault Tolerance

• Since the ConfigMap type doesn't exist in the services.yaml file, it would be a good idea to tell the user to add the object type first before talking about the fields/flags
  

• The above command returns 15, indicating that a total of 15 requests are made to the system service. Since MicroProfile’s Retry policy is disabled, only Istio’s retries are performed. Since Istio's retries was set to a max of 3, maybe explain why there resulted in 15 retries, this confused me.

Tearing down your environment

• Reference the command blocks by stating ... by running the following command: after the command description, i.e. Delete your resources from the cluster, by running the following command:, do this with all command blocks.

• the result of the kubectl label namespace default istio-injection- command is shown below, it doesn't indicate that it was deleted... is this accurate?
  

• The flow here is confusing, maybe add a paragraph before the OS specific commands to explain that you need to run this to do something. It is slightly confusing to figure out that everyone is supposed to run the topmost command as well as their OS specific command
  

• address YK comments
• subsection "Building and running the application"
  • the following statement should be moved earlier when count
    • Replace [system-pod-name] with the pod name associated with your system service, which you previously saw when running the kubectl get pods command.
• section "Enabling Istio fault tolerance"
  • add istio retries for system-virtual-service too, set it to 3 as default, hotspot for retries should highlight 2 sets of istio retries code, no need to add more description
  • "retry conditions" link not work
  • move "and wait until all of your deployments are ready and available:" before the "Pause..." statement
• section "Turning off MicroProfile Fault Tolerance except Fallback"
  • no need config.yaml file, update the services.yaml only
    • can do kubectl replace --force -f services.yaml only, i.e. not need to replace traffic.yaml
• suggest to reconsider to reword the section/subsection titles and organize them
  • existing
    • Turning off MicroProfile Fault Tolerance except Fallback
      • Setting the MP_Fault_Tolerance_NonFallback_Enabled config property
      • Adding the MicroProfile @fallback annotation
  • suggestion
    • Turning off MicroProfile Fault Tolerance
    • Using MicroProfile Fallback
• section "Tearing down your environment"
  • add a statement/phase as, "When you are done checking out the MicroProfile and Istio Fault Tolerance, you might want to teardown ..."
  • should not use kube-minikube-teardown.adoc, the instructions should be combined and use 3 different OS tabs
    • see OpenLiberty/guide-istio-intro#77

• Dockerfiles should have FROM openliberty/open-liberty:kernel-java8-openj9-ubi
• need comments for inventory and system pom.xml
  • surefire: <!-- Plugin to run unit tests -->
  • LMP: <!-- Enable liberty-maven plugin -->
  • failsafe: <!-- Plugin to run functional tests -->
  • add <!-- Provided dependencies --> above microprofile dependency
  • in properties section for plugin versions, comment should be <!-- Plugin versions -->
  • in properties section <!-- Open Liberty runtime --> should be
    <!-- Liberty configuration -->
• diff in spacing between
  • finish and start system/src/main/liberty/config/server.xml
  • finish and start inventory/src/main/java/io/openliberty/guides/inventory/client/SystemClient.java

Enabling MicroProfile Fault Tolerance

• Use the old sentence talking about the dependency in the pom.
  • do it like how the other microprofile guides did it, by saying that the API is included in the microprofile dependency
  • e.g. https://github.com/OpenLiberty/guide-microprofile-config/pull/115/files#diff-7bd4a925c695c2eb0eced3872b9b965fL109

• Readme copyright should be 2019 not 2018 2019
• Title -> "Running fault-tolerant microservices with Istio Retry and MicroProfile Fallback"
• Main Description -> "Explore ... to microservices."
• :page description: -> "Explore how to use MicroProfile and Istio Fault Tolerance"'
• :common-includes: -> ../guides-common/
• In "What you’ll learn" section:
  • change the link MicroProfile to MicroProfile Retry > "https://download.eclipse.org/microprofile/microprofile-fault-tolerance-2.0/microprofile-fault-tolerance-spec.html#retry"
  • add a link Fallback > "https://download.eclipse.org/microprofile/microprofile-fault-tolerance-2.0/microprofile-fault-tolerance-spec.html#fallback"
  • change the link Istio to Istio Retry > "https://istio.io/docs/concepts/traffic-management/#timeouts-and-retries"
• No need the "Preparing your cluster and deploying Istio" section. Promote the subsections "Starting and preparing your cluster for deployment" and "Deploying Istio" to main sections.
• In "Enabling MicroProfile fault tolerance" section:
  • "Next, the SystemClient.java file ..." -> "The SystemClient.java file ..."
  • The statements "To simulate that ...500 status code." should be moved to other place.
  • Reword the statement "The getPropertiesHelper() in SystemClient.java checks for status codes that are not OK, i.e. the status code is not 200, and throws an IOException."
  • SystemClient.java
    • buildUrl() no need the javadoc comment, simple comment is fine
    • in the last few lines the character ; no good at first of a new line (probably it is wrapped)
  • InventoryResource.java
    • the line public Response getPropertiesForHost ....throws IOException { too long
• In the subsection "Adding the MicroProfile @Retry annotation"
  • move the first 2 paragraphs after the Update command paragraph
• in the subsection "Building and running the application"
  • remove the first 2 paragraphs. It can start from "Navigate to the start directory ..."
  • should not use the docker maven plugin, should add docker build command to build the docker images
  • rename the istio.yaml to a better name, e.g. services.yaml
  • for the curl, may need to mention Postman for the first curl command, see how the Istio guide do
  • for the second time and later of "To see the number of times that..." paragraph can be shortened
  • the 2 paragraphs to explain 13 is too long. Make the explanation simple and clear
• in the "Enabling Istio fault tolerance" section
  • for the second time and later of "Pause..." paragraph can be shortened
  • the 2 paragraphs to explain 60 is too long. Make the explanation simple and clear
  • the paragraph "To prevent conflicts ..." may not need because it mentioned in the next section
• in the subsection "Conflict with MicroProfile Retry and Istio Retry"
  • Make the explanation simple and clear, no need to explain 60 again
  • the section title not right, it is not conflict but dup effort on retry
• in the subsection "Setting the MP_Fault_Tolerance_NonFallback_Enabled config property"
  • "will switch off" -> "will be switched off"
• no need config.yaml file, can do in the istio.yaml(services.yaml)
• the 2 paragraphs to explain 15 is too long. Make the explanation simple and clear
• test section is not needed
• in the pom.xml files, change <artifactId>kube-demo</artifactId> to <artifactId>microprofile-istio-retry</artifactId>

In general, the content is too much, some explanations are too detail, and repeat/similar commands can be reduced.

Peer Review: review to be done by a peer member.

Functionality

• Check that the guide has working instructions and sample code
• Test the guide on 3 platforms: Mac, Win, Linux
• If any URL visits, try them on different browsers: Firefox, Chrome, Safari
• If any URL visits, try curl command where applicable

Formatting & Presentation

• Check the quality of code according to the best coding practices
  • Ensure that command substitutions use the $(cmd) format and not `cmd`
  • Ensure that the console output of the integrated tests do not include the [INFO] tab
• Check the quality and presentation of guide according to Structure and Style Guideline
• Check the consistency of guide with template and other guides
• Examples with the right outcomes are provided
• Example with the wrong outcomes, if any, are provided
• Check that all licensing statements are properly stated in all files
• Check that the pom.xml, server.xml, etc files are clean
• Check that the directories are properly structured
• Ensure that the guide is using the latest version of the liberty-maven-plugin or liberty-gradle-plugin where applicable
• Check that some of these page-tags are used in a guide: MicroProfile, Maven, Docker, Kubernetes, Gradle, Java EE, Security, Cloud. Only these tags are visible on the website. Latest list here.
• Check the attribution statement is accurate for the guide
• Verify the travisTest.sh script, if any, is accurate and consistent with other guides

Peer Testing: tests to be done by a peer member.

Guide’s contributor’s (if available, otherwise peer tester’s) responsibility:

• Additional tests where applicable:
  • Define test coverage and review with team (including guide contributor, if available)
  • Define detail test cases
  • Consider corner cases targeting the specific guide
  • Consider different platform tests, ie, Mac, Windows, Linux
  • Consider corner cases UI tests
  • Consider testing URL on all browsers, ie, FF, Chrome, Safari
  • Consider testing the curl command for URL visits
• Consider building with both Maven and Gradle build tools
• Testing with different IDEs, ie, Atom, Eclipse (Optional: VS.code, IntelliJ, Microclimate)
• Run Acrolinx Checker on draft (above 70 score approximately)
• Consider SEO title and description for the guides
• Ensure automated test is enable, set up with Travis CI, and able to schedule tests
• Run diff -r start/ finish/ and there's no differences
• Ensure that the automation tests are able to run when PR is created

Peer Tester’s responsibility:

• Check the appearance of the guide on test site for the following items:
  • Table of contents
  • Headings
  • Paragraphs
  • code snippets
  • outputs
  • links
  • hotspots
• Test the guide end-to-end with working instruction and sample code
• Perform all the defined test cases

• after force replace the yaml, the system pod name will be regenerated, so need to ask readers to run kubectl get pods again to get the new [system-pod-name]
• at "Turning off MicroProfile Fault Tolerance" section, the first hotspot ConfigMap under the Update command should highlist the whole section instead of one line

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: inventory-config
data:
  MP_Fault_Tolerance_NonFallback_Enabled: "false" 

User Review:

Allow a person with a fresh mind to read the guide and execute the steps described in the guide, test whether each step is clear enough to understand, and no logic errors.

Review and walkthrough by a non-SME or a new user with a critical and fresh pair of eyes:

• Record the time it takes to go through the entire guide
• Test each step in the guide is clear and easy to understand
• Ensure no logic errors
• Report anything that does not work
• Report anything usability issues (confusing or hard to follow)
• Achieve basic understanding/working knowledge of the topic in the guide

Istio deployment now uses istioctl