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

make deleteWithObjects() work with empty containers #490

Merged
merged 2 commits into from
Dec 9, 2014

Conversation

joshrencher
Copy link
Contributor

As is, deleteWithObjects() will fail if container is EMPTY. This is because an object count of zero means $secondsToWait = 0, which means $endTime = time(), which means WHILE loop never runs. Quickest solution was to change "time() < $endTime" to "time() <= $endTime" on line 192, but it makes more sense to just delete the container if it's empty.

As is, deleteWithObjects() will fail if container is EMPTY. This is because an object count of zero means $secondsToWait = 0, which means $endTime = time(), which means WHILE loop never runs. Quickest solution was to change "time() < $endTime" to "time() <= $endTime, but it made more sense to just delete the container if it's empty.
@joshrencher joshrencher changed the title Update Container.php make deleteWithObjects() work with empty containers Dec 6, 2014
// Container has been deleted
// If container is empty, just delete it
$numObjects = (int) $this->retrieveMetadata()->getProperty('Object-Count');
if ($numObjects === 0) $response = $this->delete();
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than this if-else construction, you can just return early from inside the if. So something like this:

if ($numObjects === 0) {
    return $this->delete();
}

// If timeout (seconds to wait) is not specified by caller, try to
// estimate it based on number of objects in container
// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even awesomer!

On Sun, 07 Dec 2014 14:28:28 -0800, Shaunak Kashyap
[email protected] wrote:

In lib/OpenCloud/ObjectStore/Resource/Container.php:

  • // Attempt to delete all objects and container
  • $endTime = time() + $secondsToWait;
  • $containerDeleted = false;
  • while ((time() < $endTime) && !$containerDeleted) {
  • $this->deleteAllObjects();
  • try {
  • $response = $this->delete();
  • $containerDeleted = true;
  • } catch (ContainerException $e) {
  • // Ignore exception and try again
  • } catch (ClientErrorResponseException $e) {
  • if ($e->getResponse()->getStatusCode() == 404) {
  • // Container has been deleted
  • // If container is empty, just delete it
  • $numObjects = (int)
    $this->retrieveMetadata()->getProperty('Object-Count');
  • if ($numObjects === 0) $response = $this->delete();

Rather than this if-else construction, you can just return early from
inside the if. So something like this:

if ($numObjects === 0) {
return $this->delete();
}

// If timeout (seconds to wait) is not specified by caller, try to
// estimate it based on number of objects in container
// ...

Reply to this email directly or view it on GitHub [1].

Links:

[1]
https://github.com/rackspace/php-opencloud/pull/490/files#r21429173

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, once you send in that fix to this PR, the build should pass on Travis CI and we should be good to merge after that!

1) use getObjectCount() to check container size, 2) return early if empty container deleted, 3) conditional syntax changed for PSR-2 compliance, and 4) removed "and all its objects" from container exception message since we can assume objects in container were successfully deleted by deleteAllObjects();
@ycombinator
Copy link
Contributor

I'd like to make a couple of minor tweaks to this patch, including the necessary changes for the build to pass. I'm going to accept this PR in its current state and make the changes as part of a new PR.

ycombinator added a commit that referenced this pull request Dec 9, 2014
make deleteWithObjects() work with empty containers
@ycombinator ycombinator merged commit 0888361 into rackspace:working Dec 9, 2014
@ycombinator ycombinator mentioned this pull request Dec 9, 2014
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.

3 participants