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.

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

Jump to bottom

ft: ZENKO-1019 delete scheduled resume #405

Merged
merged 2 commits into from
Aug 23, 2018
Merged

ft: ZENKO-1019 delete scheduled resume #405

merged 2 commits into from
Aug 23, 2018

Conversation

philipyoo
Copy link
Contributor

@philipyoo philipyoo commented Aug 22, 2018 •
edited
Loading

Looking to merge this for pre-GA build

  • Add a DELETE route to delete/cancel a scheduled resume for given site(s)
  • Small refactor to reuse a test helper method for both POST and DELETE requests

@bert-e
Copy link
Contributor

bert-e commented Aug 22, 2018

Hello philipyoo,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get
information on this process.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Aug 22, 2018

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • 2 peers

Peer approvals must include at least 1 approval from the following list:

@rahulreddy
Copy link
Collaborator

@rachedbenmustapha This may be of interest to you. Did you already implement a cancel by sending an {} body from Orbit?

@rachedbenmustapha
Copy link
Contributor

@alexanderchan-scality can you confirm that the immediate resume is using an empty object as a body placeholder already?

@philipyoo philipyoo force-pushed the feature/ZENKO-1019-cancelScheduledResume branch from 032175d to d392585 Compare August 22, 2018 17:31
@alexanderchan-scality
Copy link
Contributor

alexanderchan-scality commented Aug 22, 2018 •
edited
Loading

@rachedbenmustapha Yes, the immediate resume/pause requests have empty bodies. As for @rahulreddy 's question about {} for the schedule requests, an empty body will default to scheduling a resume in 6 hours, so the empty will not unset the scheduled times.

JianqinWang
JianqinWang suggested changes Aug 22, 2018
View reviewed changes
lib/api/BackbeatAPI.js Outdated
@@ -736,6 +736,9 @@ class BackbeatAPI {
}
try {
reqBody = JSON.parse(body);
if (reqBody.cancel) {
Copy link
Contributor

@JianqinWang JianqinWang Aug 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If a user set this to 'false', wouldn't this skip?
In the documentation it says:
"Providing a POST request body object with a cancel key with any value will remove any scheduled resumes for the given destination replication endpoint."
We probably shouldn't give users this much freedom of usage + interpretation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good catch! I think I'm going to change this so rather than any value, I expect user to send true. Seems sort of weird to just accept any value from a ux perspective

bennettbuchanan
bennettbuchanan approved these changes Aug 22, 2018
View reviewed changes
docs/crr-pause-resume.md Outdated
@@ -141,6 +142,20 @@ is resumed, it again resumes consuming entries from its last offset.
}
```

Providing a POST request body object with a `cancel` key with any value
Copy link

Choose a reason for hiding this comment

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

Looks like it has to be a truthy value, rather than any value.

@philipyoo philipyoo force-pushed the feature/ZENKO-1019-cancelScheduledResume branch from d8b0b6f to d33c708 Compare August 22, 2018 19:04
bennettbuchanan
bennettbuchanan reviewed Aug 22, 2018
View reviewed changes
lib/api/BackbeatAPI.js Outdated
if (reqBody.cancel !== true) {
return {
error: errors.MalformedPOSTrequest.customizeDescription(
`${msg}: cancel key found with incorrect value`),
Copy link

Choose a reason for hiding this comment

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

I think it would be worthwhile to include in the message that the cancel property key must be true.

@philipyoo philipyoo force-pushed the feature/ZENKO-1019-cancelScheduledResume branch from d33c708 to 1f751dc Compare August 22, 2018 19:18
@philipyoo
Copy link
Contributor Author

Sorry instead of cancel via POST, should be a DELETE request with no body

@rahulreddy
Copy link
Collaborator

It's good to be inline with the REST API semantics 💯

@philipyoo philipyoo mentioned this pull request Aug 22, 2018
Merged
@philipyoo philipyoo force-pushed the feature/ZENKO-1019-cancelScheduledResume branch 2 times, most recently from a04cea3 to 9d071e6 Compare August 22, 2018 23:46
@philipyoo philipyoo force-pushed the feature/ZENKO-1019-cancelScheduledResume branch 2 times, most recently from 26aa43d to 7294787 Compare August 23, 2018 00:24
@philipyoo philipyoo mentioned this pull request Aug 23, 2018
Merged
@philipyoo philipyoo force-pushed the feature/ZENKO-1019-cancelScheduledResume branch from 7294787 to 21d202d Compare August 23, 2018 23:31
@philipyoo philipyoo changed the title ft: ZENKO-1019 cancel scheduled resume ft: ZENKO-1019 delete scheduled resume Aug 23, 2018
@philipyoo
ft: ZENKO-1019 add delete for scheduled resume
f2237d4 
Add a DELETE route to remove a schedule resume
@philipyoo
rf: rename and reuse http request method
3b90ad9 
Rename and reuse the http request helper method for use
with both POST and DELETE requests
@philipyoo philipyoo force-pushed the feature/ZENKO-1019-cancelScheduledResume branch from 21d202d to 3b90ad9 Compare August 23, 2018 23:33
@scality scality deleted a comment from philipyoo Aug 23, 2018
@bert-e
Copy link
Contributor

bert-e commented Aug 23, 2018

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/8.0

The following branches will NOT be impacted:

  • development/7.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of Release Engineering will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of Release Engineering now.

@bert-e
Copy link
Contributor

bert-e commented Aug 23, 2018

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/8.0

The following branches have NOT changed:

  • development/7.4

Please check the status of the associated issue ZENKO-1019.

Goodbye philipyoo.

@bert-e bert-e merged commit 3b90ad9 into development/8.0 Aug 23, 2018
@philipyoo philipyoo deleted the feature/ZENKO-1019-cancelScheduledResume branch September 7, 2018 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennettbuchanan bennettbuchanan bennettbuchanan approved these changes

@dora-korpar dora-korpar dora-korpar approved these changes

@alexanderchan-scality alexanderchan-scality alexanderchan-scality approved these changes

@JianqinWang JianqinWang JianqinWang approved these changes

@rahulreddy rahulreddy rahulreddy approved these changes

Assignees
No one assigned
Labels
high priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@philipyoo @bert-e @rahulreddy @rachedbenmustapha @alexanderchan-scality @JianqinWang @bennettbuchanan @dora-korpar