Waiters #292
Waiters #292
Conversation
| rescue StandardError => e | ||
| error = e | ||
| end | ||
| resp_or_error = resp || error |
There was a problem hiding this comment.
I should be able to assume that either one or the other will be guaranteed to be non nil?
There was a problem hiding this comment.
Yes, but this should be output_or_error I think. (Operations return an output but in v3 we called that a response. I'm not sure if I like that and maybe we can change it but let's continue to be consistent). Raise response errors should maybe be disabled instead of having this begin rescue block. We can think about that.
| end | ||
| expect_any_instance_of(waiter).to receive(:delay).and_wrap_original do |m, *args| | ||
| delay = m.call(*args) | ||
| expect(delay).to equal(5) |
There was a problem hiding this comment.
This test case is an example of the question I had in my doc regarding waiter retries. Because the remaining time (5) minus the delay (value between min delay of 3 and max delay of 4) is less than the min delay, the final delay value is set to be equal to the remaining time, which is 5. However, this exceeds the max delay. Is this expected behavior?
| rescue StandardError => e | ||
| error = e | ||
| end | ||
| resp_or_error = resp || error |
There was a problem hiding this comment.
Yes, but this should be output_or_error I think. (Operations return an output but in v3 we called that a response. I'm not sure if I like that and maybe we can change it but let's continue to be consistent). Raise response errors should maybe be disabled instead of having this begin rescue block. We can think about that.
| return false unless error.nil? && @input | ||
|
|
||
| data = { | ||
| input: @input, ### Where do we get this? |
There was a problem hiding this comment.
What do you mean? It looks to be set on the initial call. Is this what we do in v3?
There was a problem hiding this comment.
I think this was a comment I left earlier for myself to figure out where to get the input since in your Smithy-Ruby Java waiters PR you used middleware and I wasn't sure what that was/if that was necessary for Decaf. Also in V3 there's no input output matcher, only a path matcher.
| module Smithy | ||
| module Client | ||
| module Waiters | ||
| module Errors |
There was a problem hiding this comment.
Not sure yet but I think we should stuff these under smithy client errors.
There was a problem hiding this comment.
Are all errors currently defined there? I see an error for paginators in there but I also notice that errors are in other places too, like networking errors in the networking_error.rb file or CapacityNotAvailableError in client_rate_limiter.rb.
| def call(client, params) | ||
| @input = params | ||
| begin | ||
| output = client.send(@operation_name, params) |
There was a problem hiding this comment.
We should consider supporting raise response errors as a request option. Then you don't need to rescue here.
There was a problem hiding this comment.
How would I do that? I see in V3 we remove the RaiseResponseErrors handler and I tried that here but an error is still raised.
| def matches_errorType?(path_matcher, output, error) | ||
| return false unless output.nil? | ||
|
|
||
| err = path_matcher.split('#').last.split('#').first |
There was a problem hiding this comment.
Is this an absolute shape id? We should consider preprocessing this.
There was a problem hiding this comment.
This can be an absolute or relative shape id I think.
| it 'returns when successful' do | ||
| output = {} | ||
| expect(client).to receive(:get_operation).and_return(output) | ||
| expect { |
There was a problem hiding this comment.
Do .. end for multiline blocks
Or you can put it on one line and start the next line with .to
| }.to raise_error(no_such_waiter_error) | ||
| end | ||
|
|
||
| it 'does not allow custom waiters' do |
There was a problem hiding this comment.
Interesting. While true, I guess this documents that? I don't think it's needed but you can keep it if you want. I did not add one for paginators.
There was a problem hiding this comment.
Depending on whether or not we want to support registering new custom waiters this test case may be modified or removed.
| }.to raise_error(unexpected_error) | ||
| end | ||
|
|
||
| it 'raises an error for nonexistent waiters' do |
There was a problem hiding this comment.
I think this test file really just needs 2 or 3 tests. One for trying to get a non existent waiter (failure), a "good case" that doesn't raise an error, and one that verifies the return type. Is there any requirement on what waiters should return? What happens in v3? I think we possibly want to always return nil or like kernel.sleep we should return the time we waited.
There was a problem hiding this comment.
I actually had this same question earlier but I assumed the return type would either be the response or there would be an error raised based on the V3 behavior I saw. However, I don't think the return type is mentioned in the SEP or in the Smithy docs. Do any customers expect a waiter to return something? I would feel that most are using waiters to watch for the state of a resource and not really the response of an operation.
| describe 'Client: Waiters' do | ||
| let(:input) { { string_property: 'input_string' } } | ||
| let(:client) { Wait_Service::Client.new(stub_responses: true) } | ||
| let(:unexpected_error) { Smithy::Client::Waiters::Errors::UnexpectedError } |
There was a problem hiding this comment.
Nit: unless you're using these in more than one place, it's more readable to stick into the test itself.
| module Client | ||
| module Waiters | ||
| describe Waiters do | ||
| let(:shapes) do |
There was a problem hiding this comment.
I was hoping this would be slimmed down. We don't need to define all of the possibilities up front and we certainly don't need documentation traits etc. This model would be difficult to maintain.
I think we would want context blocks and make small services that have each concrete example.
Let's talk offline.
There was a problem hiding this comment.
These shapes is a lot to scroll through - might be better to put these in a separate json file?
| module Client | ||
| module Waiters | ||
| describe Waiters do | ||
| let(:shapes) do |
There was a problem hiding this comment.
These shapes is a lot to scroll through - might be better to put these in a separate json file?
Description of changes:
Initial implementation of waiters.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.