Skip to content

issue-7-stop-job #146

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

Closed
viveksachdeva wants to merge 5 commits into from
Closed

issue-7-stop-job #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
375a5b1
issue-3 stop option added to servlet
Aug 24, 2016
a886ebe
added option to stop running job
Aug 24, 2016
9044db3
updated readme
Aug 24, 2016
f94a8f1
changed stop job option to delete instead of POST
Aug 29, 2016
4524734
unused import
Aug 29, 2016
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
5 changes: 5 additions & 0 deletions docs/Running.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ image::../assets/monitor.png[Monitor Jobs]

See link:Monitoring.adoc[Monitoring / Validating the Content Sync] for information on evaluating the returned information.

=== Stopping a Job

From the main screen, enter "s" to stop an existing job. You will be prompted to enter job id.

Enter the job id that you wish to terminate. That job will then be stopped.

==== Configuration

Expand Down
18 changes: 17 additions & 1 deletion grabbit.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,20 @@ function monitorStatus {
done
}

function stopJob {
echo "Enter jobID to stop, or \"b\" to go back"
read selection
if [ "$selection" == "b" ]; then
echo "*****************************************************"
return
fi
statusJson=`curl -s -u $username:$password --request DELETE $client$GRABBIT_JOB"?jobId="$selection`
echo
echo "$statusJson"
echo "*****************************************************"
}


clear
echo $SET_BLUE
echo "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"
Expand All @@ -105,12 +119,14 @@ echo "Client Password :"
read -s password

while true; do
echo "Enter \"n\" for new job request, \"m\" to monitor, or \"q\" to quit"
echo "Enter \"n\" for new job request, \"m\" to monitor, \"s\" to stop a job, or \"q\" to quit"
read selection
if [ "$selection" == "n" ]; then
newGrabbitRequest
elif [ "$selection" == "m" ]; then
monitorStatus
elif [ "$selection" == "s" ]; then
stopJob
elif [ "$selection" == "q" ]; then
exit 0;
else
Expand Down
34 changes: 31 additions & 3 deletions src/main/groovy/com/twcable/grabbit/client/servlets/GrabbitJobServlet.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ import org.apache.sling.api.SlingHttpServletRequest
import org.apache.sling.api.SlingHttpServletResponse
import org.apache.sling.api.servlets.SlingAllMethodsServlet
import org.springframework.batch.core.explore.JobExplorer
import org.springframework.batch.core.launch.support.SimpleJobOperator
import org.springframework.context.ConfigurableApplicationContext

import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST
import static javax.servlet.http.HttpServletResponse.SC_OK
import static javax.servlet.http.HttpServletResponse.*

/**
* This servlet is used to manage Grabbit jobs.
Expand All @@ -49,7 +49,7 @@ import static javax.servlet.http.HttpServletResponse.SC_OK
*/
@Slf4j
@CompileStatic
@SlingServlet(methods = ['GET', 'PUT'], resourceTypes = ['twcable:grabbit/job'])
@SlingServlet(methods = ['GET', 'PUT', 'DELETE'], resourceTypes = ['twcable:grabbit/job'])
class GrabbitJobServlet extends SlingAllMethodsServlet {

//A "special" meta-jobID that allows for the status of all jobs to be queried.
Expand Down Expand Up @@ -147,6 +147,34 @@ class GrabbitJobServlet extends SlingAllMethodsServlet {
response.writer.write(new JsonBuilder(jobExecutionIds).toString())
}

@Override
protected void doDelete(SlingHttpServletRequest request, SlingHttpServletResponse response) {
String jobExecutionId = request.getParameter("jobId") ?: ""

if(jobExecutionId == ALL_JOBS_ID) {
response.setStatus(SC_BAD_REQUEST)
response.writer.write("Stopping 'all' jobs is not supported. Please specify single job id")
return
}
if(!jobExecutionId.isLong()) {
log.warn "Parameter ${jobExecutionId} 'jobId' is invalid"
Copy link
Copy Markdown
Member

@jbornemann jbornemann Aug 24, 2016

Choose a reason for hiding this comment

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

Hmm..for the case of "all", wondering if we should provide different messaging than saying it's invalid. Really it's valid, just not supported for DELETE/POST

Copy link
Copy Markdown
Author

@viveksachdeva viveksachdeva Aug 26, 2016

Choose a reason for hiding this comment

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

@jbornemann : did not get this point... job id parameter needs to be sent to stop that job.. what is 'all' in this case?

Copy link
Copy Markdown
Member

@jbornemann jbornemann Aug 26, 2016

Choose a reason for hiding this comment

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

Sure. Take a look at https://github.com/TWCable/grabbit/blob/master/src/main/groovy/com/twcable/grabbit/client/servlets/GrabbitJobServlet.groovy#L56

You can use "all" as a special meta-parameter to query all known jobs, e.g:

GET /grabbit/job/all

So what I'm saying here is - if someone tries to

DELETE /grabbit/job/all

we should not tell them that the jobID 'all' is invalid, because it's not. We should tell them deleting 'all' is unsupported or something

Copy link
Copy Markdown
Author

@viveksachdeva viveksachdeva Aug 26, 2016

Choose a reason for hiding this comment

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

ahh... that one.. ok

response.status = SC_BAD_REQUEST
response.writer.write("Parameter 'jobId' is invalid")
return
}
try {
Copy link
Copy Markdown
Member

@jbornemann jbornemann Aug 24, 2016

Choose a reason for hiding this comment

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

SimpleJobOperator jobOperator = configurableApplicationContext.getBean(SimpleJobOperator)
try {
  jobOperator.stop(jobExecutionId.toLong())
}
catch(NoSuchJobExecutionException ex) {
  response.status = SC_NOT_FOUND
  response.writer.write("No such job exists with id ${jobExecutionId}")
}
catch(JobExecutionNotRunningException ex) {
  response.status = SC_OK
  response.writer.write("Job already complete for id ${jobExecutionId}. Nothing to stop")
}

Copy link
Copy Markdown
Author

@viveksachdeva viveksachdeva Aug 26, 2016

Choose a reason for hiding this comment

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

@jbornemann : This is also something that I had earlier but changed it to single exception block... Reason I changed it:
It only looks for in memory jobs... So there could be case when job is there in the grabbit repository and we run stop for that particular id and we would get "No Such job exists".. And as stop() can only throw these 2 exceptions, I moved those to single block with one message..

Copy link
Copy Markdown
Member

@jbornemann jbornemann Aug 26, 2016

Choose a reason for hiding this comment

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

@viveksachdeva I'm not sure what you mean. All jobs, including ones being run are backed by the grabbit repository.

I think we should have the distinction...and at the very least do away with catching "Exception" for reasons I've mentioned before. It will just save us from having to clean this up in the future, because the Checker framework will complain about it..

Copy link
Copy Markdown
Author

@viveksachdeva viveksachdeva Aug 26, 2016

Choose a reason for hiding this comment

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

@jbornemann : Lets say I have lots of jobs in grabbit repo that were run some time in past and after that instance was restarted, then if I say stop job on one of those jobs, it will give throw NoSuchJobExecutionException instead of JobExecutionNotRunningException...
If we decide to remove those jobs in addition to stopping, then two exception blocks would be more useful...

Copy link
Copy Markdown
Member

@jbornemann jbornemann Aug 26, 2016

Choose a reason for hiding this comment

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

Can you please confirm that is the case, because that is contrary to the documentation : http://docs.spring.io/spring-batch/apidocs/org/springframework/batch/core/launch/JobOperator.html#stop-long-

In your scenario, the jobExecutionId would still be present within the job repository; So when Spring Batch queries the repository, it should not return NoSuchJobExecutionException..

In your scenario, I would expect JobExecutionNotRunningException..

Copy link
Copy Markdown
Author

@viveksachdeva viveksachdeva Aug 29, 2016

Choose a reason for hiding this comment

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

Checked it again with older jobs.. It throws NoSuchJobException

SimpleJobOperator jobOperator = configurableApplicationContext.getBean(SimpleJobOperator)
jobOperator.stop(jobExecutionId.toLong())
Copy link
Copy Markdown
Contributor

@sagarsane sagarsane Aug 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

We should also look to see whether any other cleanup of like the inputstream / session is needed after stopping the Job.

Take a look at ClientBatchJobExecutionListener.afterJob()

Also, this will only stop the client .. server will still be wasting time and resources sending his content over :-(

Copy link
Copy Markdown
Member

@jbornemann jbornemann Aug 26, 2016

Choose a reason for hiding this comment

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

Oh wow, yeah, good catch.

Copy link
Copy Markdown
Author

@viveksachdeva viveksachdeva Aug 29, 2016

Choose a reason for hiding this comment

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

@sagarsane : afterJob is getting executed automatically on trigger of stop()..

response.status = SC_OK
response.writer.write("Job ${jobExecutionId} Successfully Stopped")
}
catch (Exception exception){
response.status = SC_NOT_FOUND
response.writer.write("Could not find a running job with id ${jobExecutionId}")
}

}

/**
* Will return the status of a job from the {@link org.springframework.batch.core.explore.JobExplorer} used in JSON format.
* @param jobId The jobID to get status.
Expand Down
39 changes: 37 additions & 2 deletions src/test/groovy/com/twcable/grabbit/client/GrabbitJobServletSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ import org.apache.sling.api.resource.Resource
import org.apache.sling.api.resource.ResourceMetadata
import org.springframework.batch.core.JobParameters
import org.springframework.batch.core.explore.JobExplorer
import org.springframework.batch.core.launch.JobExecutionNotRunningException
import org.springframework.batch.core.launch.NoSuchJobException
import org.springframework.batch.core.launch.support.SimpleJobOperator
import org.springframework.context.ConfigurableApplicationContext
import spock.lang.Specification
import spock.lang.Subject
Expand All @@ -36,8 +39,7 @@ import javax.annotation.Nonnull
import javax.servlet.ServletInputStream

import static com.twcable.grabbit.client.servlets.GrabbitJobServlet.ALL_JOBS_ID
import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST
import static javax.servlet.http.HttpServletResponse.SC_OK
import static javax.servlet.http.HttpServletResponse.*

@Subject(GrabbitJobServlet)
class GrabbitJobServletSpec extends Specification {
Expand Down Expand Up @@ -234,6 +236,39 @@ class GrabbitJobServletSpec extends Specification {
//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"() {
given:
SlingHttpServletRequest request = Mock(SlingHttpServletRequest) {
getParameter("jobId") >> inputJobId
}
SlingHttpServletResponse response = Mock(SlingHttpServletResponse) {
getWriter() >> Mock(PrintWriter)
}
GrabbitJobServlet servlet = new GrabbitJobServlet()
final applicationContext = Mock(ConfigurableApplicationContext) {
getBean(SimpleJobOperator) >> Mock(SimpleJobOperator) {
stop(123L) >> true
stop(222L) >> {throw new NoSuchJobException("Job does not exist")}
stop(333L) >> {throw new JobExecutionNotRunningException("Job not running")}
}
}
servlet.setConfigurableApplicationContext(applicationContext)

when:
servlet.doDelete(request, response)

then:
1 * response.setStatus(expectedResponse)

where:
inputJobId | expectedResponse
123L | SC_OK
222L | SC_NOT_FOUND
333L | SC_NOT_FOUND
"aaa" | SC_BAD_REQUEST
"" | SC_BAD_REQUEST
}

class StubServletInputStream extends ServletInputStream {


Expand Down