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

Adding Redshift waiter: ClusterRestored #836

Merged
merged 1 commit into from
Jun 11, 2015

Conversation

slpsys
Copy link

@slpsys slpsys commented Jun 5, 2015

While scripting some Redshift operations, I found that the existing set of waiters only includes :cluster_available, which returns immediately when called for all three of these clusters:

irb(main):029:0> pp aws[:redshift].describe_clusters.clusters.map { |c| [c.cluster_identifier, c.cluster_status, c.restore_status.try(:status)] }
[["science-20150603-11", "available", "completed"],
 ["science-20150605-12", "available", "restoring"],
 ["warehouse03", "available", nil]]

whereas I actually want it to continue waiting for the second (["science-20150605-12", "available", "restoring"]. This PR adds a waiter for ClusterRestored, which will wait for the restore status to also become 'completed'.

One question(...? thought?) I did have: obviously implicit in this is that restore_status.status can only ever become 'completed' after cluster_status becomes 'available'. This seems to be a reasonable assumption, but seems brittle. Dunno how you feel about that. Possible future approaches that I didn't see but may be planned, being worked on, or exist, would be either multiple criteria per edge in the state graph or waiter composition.

Some contributing-related notes:

  • Totally fine if a different (more specific?) waiter name is in order
  • There aren't any non-config changes, so it's passing tests, but I didn't see any tests that cover waiters specifically

trevorrowe added a commit that referenced this pull request Jun 11, 2015
Adding Redshift waiter: ClusterRestored
@trevorrowe trevorrowe merged commit 9f98245 into aws:master Jun 11, 2015
@trevorrowe
Copy link
Member

Thanks, this should be part of our 2.1 release shortly.

@trevorrowe
Copy link
Member

Sorry, submitted to soon. I'm fine with the name. It is description enough. The waiter could be updated in the future if needed to be more specific with respect to the cluster status if that becomes a problem. Something that we could investigate is making the path expression return the compound value.

{
  "state": "success",
  "matcher": "pathAll",
  "argument": "Clusters[].[ClusterStatus,RestoreStatus.Status]",
  "expected": ["available", "completed"]
},

I would have to discuss this change with the other SDK maintainers, but it seems sensible. This could future-proof the waiter against other cluster statuses with a restore status of completed, however unlikely.

Currently we do not integration test these waiters as the setup would be pretty difficult. Instead, when I merge these up-stream, there is a validator that will ensure the path expressions are valid and will resolve. We are still looking into alternatives to ensure that the desired behavior is achieved.

@slpsys
Copy link
Author

slpsys commented Jun 11, 2015

Awesome, thanks! Yeah, that approach seems sensible to me as well, but obviously your call.

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.

2 participants