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.

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

Updates and additions for the MP RESTful web service guide #215

Merged
merged 27 commits into from
Dec 19, 2018
Merged

Updates and additions for the MP RESTful web service guide #215

merged 27 commits into from
Dec 19, 2018

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Nov 29, 2018

This PR is for the guide for the MicroProfile RESTful web app.

Romain and Joe and I met a couple weeks ago about the SE (not MP) guide. These changes do not incorporate the improvements we talked about in that meeting. I wanted to get this guide published, and then I can update the SE and MP guides together (see
#176) to reflect that meeting.

As part of this change I have added tags to the SE guide's .adoc file and include those segments into the MP guide. I have created a separate issue to think about and do some refactoring if we feel it's worthwhile to pull the common portions out to yet another file and have both guide .adoc files include from the refactored common ones.

Copy link
Contributor

@kumar-dhanagopal kumar-dhanagopal left a comment

Choose a reason for hiding this comment

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

I'd like to read through and review examples/guides/mp-restful-webservice/src/main/docs/index.adoc. By when do you need the review to be completed?

@tjquinno
Copy link
Member Author

@kumar-dhanagopal I am going to be out of the office some on Friday and some on Monday. If I aim to do the merge on Tuesday of next week is that enough time?

@kumar-dhanagopal
Copy link
Contributor

@kumar-dhanagopal I am going to be out of the office some on Friday and some on Monday. If I aim to do the merge on Tuesday of next week is that enough time?

Yes, that will give me time to do a quick review. Thank you! I'll send in my comments by CoB Monday, PT.

@tjquinno
Copy link
Member Author

One way to look at the pages live is to clone my forked repository and run a simple HTTP server locally (if I remember this correctly...):

cd to the top level of the just-cloned workspace
mvn install
mvn install -Psources
cd javadocs ; mvn generate-sources
cd ../docs ; mvn package
cd target/docs ; python -m SimpleHTTPServer 8080

Then in a browser go to http://localhost:8080 and expand the Guides section in the left-hand nav panel.

If the sequence of commands does not work please get in touch with Romain while I am out.

@tjquinno
Copy link
Member Author

tjquinno commented Dec 6, 2018

@kumar-dhanagopal Just for my own planning, do you have an ETA for review phase 2 as described in #219? Thanks.

@kumar-dhanagopal
Copy link
Contributor

kumar-dhanagopal commented Dec 6, 2018

@kumar-dhanagopal Just for my own planning, do you have an ETA for review phase 2 as described in #219? Thanks.

I'm going to try rebuilding the docs tomorrow. If that works, then I'll complete the phase-2 review by Tue next week.

@tjquinno
Copy link
Member Author

Received email from Kumar with these notes and my emailed responses:

GreetResource.java
When I added @produces(MediaType.APPLICATION_JSON), my IDE (NetBeans) prompted me to import the following packages.
image
I selected javax.ws.rs.core.MediaType as in the doc. But the IDE then displays a ‘Cannot find symbol’ error. What could be wrong?

I’m not sure. I just tried it again myself and it worked fine for me. That class is in the javax.ws.rs-api-2.1.jar (maven group javax.ws.rs, artifact javax.ws.rs-api) which is pulled in as a result of adding this dependency to the pom.xml:

    <dependency> <!--2-->
        <groupId>org.glassfish.jersey.media</groupId>
        <artifactId>jersey-media-json-binding</artifactId>
    </dependency>

which is one of the dependencies the guide describes adding. Is that in your pom?

Main class
For Main.class.getResourceAsStream("/logging.properties"));, the IDE prompted me to import from the following options:
image
None of these packages is in the doc. Which should we import?

There should be no need to import anything. The Main.class is intended to refer to the current Main class, not some other one.

pom.xml
Should we replace the element in the generated pom.xml (attached) with what’s in the doc?
Should we replace the element in the generated pom.xml, or only add the dependencies from the doc and leave the auto-generated ones as is?
Should we keep the element as is in the generated pom?

How did you create your pom, just out of curiosity?

The intent is that the pom contents in the guide are merged into whatever pom the generic Java maven archetype or your IDE created. So if what the guide says is already there, replace it; if absent, add it. I will clarify that in the guide text.

This sentence:
“If you run your project from the IDE, the IDE typically handles the main class and places dependent JARs on the runtime classpath for you and your pom is now ready to go.”
à Otherwise, i.e., for users who use a text editor for example, what instructions should we add?

There is a section in the SE guide (last part of "Update your pom") for exactly this purpose that, for brevity after some other review discussions, I left out of the MP guide. I have added the same notes into the MP guide in light of your comment.

src/main/resources
This directory didn’t exist when I ran mvn archetype:generate. Is that the expected behavior? If yes, then in Create an MP config file, we should add a step to create that directory.

I think that’s normal. I suppose we could add explicit directions for the reader to create missing directories (the developer also needs to create the directory for the MP config file) but the guide gives the full path (within the maven project) where each of these files is to reside, and to my mind it’s not asking too much for a developer to know to create a directory we specify for a file if the directory is not already there.

@kumar-dhanagopal
Copy link
Contributor

kumar-dhanagopal commented Dec 12, 2018

@tjquinno - Hi Tim, I finished reviewing (and trying out) the remainder of the MP RESTful web services guide.

  1. In the code snippets for GreetResource.java and HealthResource.java, for the benefit of users who'll copy-paste code as is, consider doing one of the following:

    • Include the closing "}" as well. This is my preference.
    • Or, omit the class definition statement (assuming the IDE or user will add it) and keep only the annotations.
  2. When introducing the packages to be imported, in most of the sections I see: "You or your IDE will add these imports along the way". I suggest that we explicitly ask users to add the import statements listed in the doc. Otherwise, when the IDE prompts them to choose from the possible packages, users might pick an incorrect package and end up spending time troubleshooting, like I was trying to do.

  3. In Build and run, we say: "...run these commands to access its three functions (order is important for the last two)". By "three functions" I guess you meant (1) get generic greeting, (2) get personalized greeting, and (3) change the greeting. But because the phrase "three functions" occurs so close to "last two" (which refers to (3) and then (2)), at the first reading, it appears that the command table lists 4 functions. To solve this problem, I suggest dropping "three". Also, consider adding a "Function" column (perhaps as the first one in the table) and state the goal of each command in that column.

  4. Regarding the line "liveness - often a more thorough assessment of if and how well the service can do its job":

    • "liveness" is an odd term. Would "health" work? If yes, pls note that the doc has multiple occurrences of "liveness", "alive" etc. I recommend changing all of them (even the /alive path in the code if possible) to "health", "healthy", etc. as appropriate.
    • Also, to avoid the awkward "of if" combination in that sentence :-), consider changing "assessment of if" to "assessment of whether". As a general practice, I recommend using "whether" and not "if" in such situations.
  5. In the sentence, "For this example we define our service as active, consider using "healthy" in place of "active".

  6. In Update GreetApplication, for the sake of clarify, I suggest adding a snippet showing the code after making this change:

@Override
    public Set<Class<?>> getClasses() {
        return CollectionsHelper.setOf(
                GreetResource.class,
                HealthResource.class
        );
  1. "Rebuild and rerun your service". The first time I saw this section, I read “Follow the same steps you did before” and then jumped ahead to rebuilding/rerunning the app, without reading the part about stopping the running app. And of course, I ran into the ‘unable to bind port’ error. To prevent users from committing this error, I suggest turning that para into 2 steps:

      1. Stop any running instance of your app.
      1. Rebuild the app and then run it.
  2. "Download the example source (optional)"

    • The convention in official docs is to write “(Optional)” first and then the heading text, i.e., "(Optional) Download the example source", so that users notice that the step is optional before they read the heading.
    • “writing the application yourself” --> consider changing to “building the application using the code snippets in this guide”

@tjquinno
Copy link
Member Author

tjquinno commented Dec 12, 2018

@kumar-dhanagopal Thanks for the further comments. I've adopted most of your suggestions. Exceptions noted below.

  1. Regarding the line "liveness - often a more thorough assessment of if and how well the service can do its job":

    • "liveness" is an odd term. Would "health" work? If yes, pls note that the doc has multiple occurrences of "liveness", "alive" etc. I recommend changing all of them (even the /alive path in the code if possible) to "health", "healthy", etc. as appropriate.
  2. In the sentence, "For this example we define our service as active, consider using "healthy" in place of "active".

"Liveness" is a term of art in the Kubernetes world (see https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/)
and "health" is the broader category that includes readiness and liveness. I used those terms in our guides specifically to align with the K8s usage.

@tjquinno
Copy link
Member Author

I have pushed my latest updates to the fork, so if you want you can pull and rebuild to inspect the changes locally.

@kumar-dhanagopal
Copy link
Contributor

"Liveness" is a term of art in the Kubernetes world (see https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/)
and "health" is the broader category that includes readiness and liveness. I used those terms in our guides specifically to align with the K8s usage.

Thanks for clarifying that, @tjquinno ! Is it reasonable/safe to assume that microservices will be deployed typically (only?) to K8s containers?

In the sentence: ""For this example we define our service as active..." could you use "alive" instead of "active?

@kumar-dhanagopal
Copy link
Contributor

I have pushed my latest updates to the fork, so if you want you can pull and rebuild to inspect the changes locally.

Sure. I have a few JCS/PSM tasks to take care of today and tomorrow. After that, I'll pull the updates, rebuild, and verify. Thank you!

@tjquinno
Copy link
Member Author

Thanks for clarifying that, @tjquinno ! Is it reasonable/safe to assume that microservices will be deployed typically (only?) to K8s containers?

Not necessarily, but I think readers will be familiar with these terms and their use in K8s.

@spericas
Copy link
Member

Thanks for clarifying that, @tjquinno ! Is it reasonable/safe to assume that microservices will be deployed typically (only?) to K8s containers?

Not necessarily, but I think readers will be familiar with these terms and their use in K8s.

I agree, developers are indeed familiar with the terminology used by K8s.

@kumar-dhanagopal
Copy link
Contributor

I have pushed my latest updates to the fork, so if you want you can pull and rebuild to inspect the changes locally.

Done. I verified the fixes in a local build. They look good!

Just one incremental comment ...

In Update your pom.xml, I think the parenthetical phrase ...

possibly in addition to others

... means that users must add the properties & dependencies listed in the doc if they don't exist, but that they can choose to leave any other existing properties & dependencies in the pom as is. If that's the intent, then I suggest making it clearer. Consider rephrasing as: "Add the properties and dependencies listed here if they don't exist in your pom.xml).

@romain-grecourt
Copy link
Contributor

romain-grecourt commented Dec 19, 2018

@kumar-dhanagopal @tjquinno Shouldn't Maven be capitalized ?

@kumar-dhanagopal
Copy link
Contributor

@kumar-dhanagopal @tjquinno Shouldn't Maven be capitalized ?

Yes, it should. Good catch!

The following lines of text should be updated:

  • Create a new maven project
  • You can create your maven project in these ways:
  • use your IDE to create a new Java maven project, or
  • run the standard maven archetype to create a new Java project using this command:
  • To run maven outside your IDE
  • If you want to use maven yourself,
  • This is typical with maven projects
  • Instructs maven what main class to set in the
  • Tells maven to package the dependency
  • Or, to use maven outside the IDE,

@tjquinno
Copy link
Member Author

@kumar-dhanagopal "Maven" spelling and instructions for revising pom.xml changed as described.

@kumar-dhanagopal kumar-dhanagopal self-requested a review December 19, 2018 21:34
@tjquinno tjquinno merged commit 2238752 into helidon-io:master Dec 19, 2018
@tjquinno tjquinno deleted the initial-mp-rest-guide branch May 24, 2019 18:45
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.

6 participants