Skip to content

Conversation

@viveksachdeva
Copy link

@viveksachdeva viveksachdeva commented Aug 24, 2016

Issue: #7

Added option to stop a job with given execution id...

Ready for review.
@jbornemann @jdigger @sagarsane @jayati-jt @Shashi-Bhushan

@review-ninja
Copy link

ReviewNinja

@Slf4j
@CompileStatic
@SlingServlet(methods = ['GET', 'PUT'], resourceTypes = ['twcable:grabbit/job'])
@SlingServlet(methods = ['GET', 'PUT', 'POST'], resourceTypes = ['twcable:grabbit/job'])
Copy link
Member

Choose a reason for hiding this comment

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

I'm kinda thinking 'DELETE' may be more appropriate in this case. What do you think @sagarsane @viveksachdeva ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that might be better...

Copy link
Author

Choose a reason for hiding this comment

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

I used DELETE initially but changed it to POST... I thought that it is kind of an update... and I think of DELETE method as something for cleanup/removal... For me, ideal method for this would be PUT but that was used..
Open to change.. Thoughts??

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I see your point.

I initially suggested the DELETE verb for removing jobs from the repository. If you remove a job from the repository, ideally you would make sure it is stopped as well (which we aren't doing currently in the clean servlet I don't think, but..); and possibly vice-versa. So, maybe we can consolidate stopping a job, and deleting a job in the future, because really having them be seperate concepts is sort of an artificial creation..

So I guess if I had to lean one way, it might be DELETE

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.

5 participants