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

Improve documentation coverage for Spring Batch #19211

Closed
wants to merge 2 commits into from
Closed

Improve documentation coverage for Spring Batch #19211

wants to merge 2 commits into from

Conversation

Buzzardo
Copy link

@Buzzardo Buzzardo commented Dec 2, 2019

At Michael Minella's suggestion and with his input, I extended the Batch section of the How-to page to include more detail.

@philwebb philwebb added this to the 2.2.x milestone Dec 2, 2019
@philwebb philwebb added the type: documentation A documentation update label Dec 2, 2019
@snicoll snicoll changed the title More detail for Batch within Boot Improve documentation coverage for Spring Batch Dec 7, 2019
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

They are a number of TODOs in this PR and I am confused as whether you're expecting us to answer those or if you're waiting for some input of the Spring Batch team.


NOTE: By default, batch applications require a `DataSource` to store job details.
* <<howto-spring-batch-specifying-a-data-source>>
Copy link
Member

Choose a reason for hiding this comment

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

We usually don't list sections like that. Our toc on the left hand side offers a list of sub-sections anyway so I think this is redundant (and likely to be outdated if we forget to update the list).

Copy link
Author

Choose a reason for hiding this comment

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

It's regarded as a best practice (part of information mapping) in the technical writing community. However, I removed it, because I have no idea how it works with the ToC being constantly present. Do readers refer to the ToC as they read? (I don't, but I'm not everyone.) I wish I had data around that, to have something other than intuition on which to base this kind of decision. Anyway, I removed it.


[[howto-execute-spring-batch-jobs-on-startup]]
=== Execute Spring Batch Jobs on Startup
[[howto-run-spring-batch-jobs-on-startup]]
Copy link
Member

Choose a reason for hiding this comment

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

If the anchor is changed, I'd rather harmonize it to the same prefix as you've done for everything else.

Copy link
Author

Choose a reason for hiding this comment

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

Got it.

They work a bit differently. However, for the most part, the differences do not cause
trouble. However, they parse command line arguments differently, which can cause trouble,
as described in the next section.
// TODO What are the other differences between the two?
Copy link
Member

Choose a reason for hiding this comment

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

Is that something we're supposed to handle?

Copy link
Contributor

Choose a reason for hiding this comment

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

The other difference is that the CommandLineJobRunner from batch can accept options like -next, -stop, -restart, etc while the one from boot does not.

Copy link
Author

Choose a reason for hiding this comment

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

I left out that detail, since I didn't think it would help a user of Spring Boot.


==== Passing Command-line Arguments

If you run Spring Batch with Spring Boot, Spring Boot strips the first `-` character (a
Copy link
Member

Choose a reason for hiding this comment

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

This section should use one sentence per line as the rest of the doc please.

Copy link
Author

Choose a reason for hiding this comment

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

I made that change.

In the second example, Spring Boot passes the parameter to Spring Batch as
`-jobParameters="someParameter"`, and "someParameter" does not become the name of the
`Job`.
// TODO In that case, what is the name of the job?
Copy link
Member

@snicoll snicoll Dec 9, 2019

Choose a reason for hiding this comment

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

I must admit I am bit confused by the current phrasing. Shouldn't we rephrase it around stating that Spring Boot uses -- as a signal for application arguments and that this clash with the way Spring Batch core works?

Copy link
Contributor

Choose a reason for hiding this comment

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

For this TODO, it is not the name of the job that is relevant, it is rather the job instance identity. When there is "-" in front of a parameter, the parameter is considered as non-identifying and will not contribute to the identity of the job instance. So the sentence could be re-phrased to something like:

In the second example, Spring Boot passes the parameter to Spring Batch as
-someParameter="someValue", and "someParameter" will be considered as a non-identifying job parameter.

Copy link
Author

@Buzzardo Buzzardo Dec 12, 2019

Choose a reason for hiding this comment

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

I wrote a paragraph at the top of this section to address Stéphane's concern. Then I made the change Mahmoud suggested.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Dec 9, 2019
@Buzzardo
Copy link
Author

I've asked Mahmoud Ben Hassine (benas here on Github) to review the TODOs. Once that is done, I'll fix it up. Unfortunately, I can't add him as a reviewer.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 10, 2019
@snicoll
Copy link
Member

snicoll commented Dec 11, 2019

Waiting for input from @benas then...

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Dec 11, 2019
https://docs.spring.io/spring-batch/trunk/reference/html/configureJob.html#configuringJobRepository[Configuring
a Job Repository]
// TODO Is it best practice to use a database (rather than the default Map object) for
// the Job Repository, even when Spring Boot isn't present?
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the map based job repository is only intended for testing (as mentioned in its javadoc).

Copy link
Author

Choose a reason for hiding this comment

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

I dropped most of this, because it doesn't apply under Spring Boot. (I originally wrote this document as a stand-alone document.)

example:

[source]
jobParameters="someParameter"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing, I would use someParameter="someValue" instead.

Copy link
Author

Choose a reason for hiding this comment

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

Good plan. I kept jobParameters when it's clear we're talking about exactly that argument.

Jay Bryant added 2 commits December 12, 2019 12:01
At Michael Minella's suggestion and with his input, I extended the Batch section of the How-to page to include more detail.
Stéphane Nicoll and Mahmoud Ben Hassine had comments on my changes. I have accounted for their input. Thanks, Stéphane and Mahmoud.
@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Dec 23, 2019
@snicoll snicoll self-assigned this Dec 23, 2019
@snicoll snicoll modified the milestones: 2.2.x, 2.2.3 Dec 23, 2019
@snicoll snicoll closed this in 4fb0a2b Dec 23, 2019
snicoll added a commit that referenced this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants