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

issue-7 stop job client #167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

viveksachdeva
Copy link

Old PR #146 had conflicts..

This one takes care of stopping jobs at client end.

@review-ninja
Copy link

ReviewNinja

@jbornemann
Copy link
Member

Great! I'll take a look at this tomorrow

Copy link
Member

@jbornemann jbornemann 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 picking this back up! This is on the right track!

There are a few things we are going to want to address :

1.) When we we stop the job from the job operator, it ceases step executions, and calls our ClientBatchJobExecutionListener.afterJob(JobExecution) as we expect to do things like close the session, and close this input stream; however, when we close this particular input stream instance, the connection is left intact until the entity (grabbit stream) is fully downloaded. This effectively means that stopping the job ceases writes to the JCR, and reads from the input stream; but the input stream continues to fill. In the case of stopping a job, we could discard the entity to avoid this. See https://hc.apache.org/httpcomponents-core-ga/httpcore/apidocs/org/apache/http/util/EntityUtils.html#consume(org.apache.http.HttpEntity).

2.) When the client closes the connection per above, the sending server will fail badly in JcrNodesWriter via a peer reset. Instead of failing the job, I think we should probably implement an approach to abandoning the server job per it's respective client job. We may be able to utilize http://docs.spring.io/spring-batch/apidocs/org/springframework/batch/core/step/tasklet/StoppableTasklet.html on the http connection tasklet for managing this clean-up. It's only available in Spring Batch 3.0, and higher though, so we would have to vet out an upgrade. A possible approach is to send back the server jobs ID in a header, save it via the client job's thread; and in the case of a job stop, call DELETE /grabbit/content?jobID= which then sets the job status abandoned. On an SC_OK, we could close the connection as normal.

Let me know if you need any help

}
catch (NoSuchJobExecutionException noSuchJobExc){
response.status = SC_NOT_FOUND
response.writer.write("No suhc job exists with id ${jobExecutionId}")
Copy link
Member

Choose a reason for hiding this comment

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

typo

@@ -232,4 +236,38 @@ class GrabbitJobServletSpec extends Specification {
inputStream << [inputStream(" "), inputStream("foo: 'foo'")]
//One causes SnakeYAML to produce a null config map, and the other does not pass our validations (missing values)
}

def "Stop job response check with differnt set of jobId parameter"() {
Copy link
Member

Choose a reason for hiding this comment

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

typo. Title is also a bit confusing. Maybe "Stopping a job with input job ID #jobID expects a response of #expectedResponse"

@jbornemann
Copy link
Member

If you are curious about how close() behaves in our case, check out org.apache.http.impl.io.ChunkedInputStream.close() (which is wrapped by our entities EofSensorInputStream class). You can see it continues to read into a 2KB buffer on close, until EOF is reached.

@sagarsane
Copy link
Contributor

This is a great start 👍 .. I agree with @jbornemann 's points as well.

@jbornemann
Copy link
Member

1ffe73f is merged, so this may help with your progress on this branch

@viveksachdeva viveksachdeva force-pushed the issue-7-implement-stop-job-client branch from 3a103e6 to 8015369 Compare January 19, 2017 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants