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

ability for waiter.wait to return last response on success #2913

Open
1 of 2 tasks
thehesiod opened this issue Apr 25, 2023 · 10 comments · May be fixed by #2914
Open
1 of 2 tasks

ability for waiter.wait to return last response on success #2913

thehesiod opened this issue Apr 25, 2023 · 10 comments · May be fixed by #2914
Labels
feature-request This issue requests a feature. p2 This is a standard priority issue waiter

Comments

@thehesiod
Copy link

Describe the feature

right now when the last item is a success it just returns None: https://github.com/boto/botocore/blob/develop/botocore/waiter.py#L370.

Use Case

This means that if you want to grab statistics, like from an athena call you have to call the method again to be able to get it.

Proposed Solution

To avoid another round trip the waiter should return the last response

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

1.29.119

Environment details (OS name and version, etc.)

OSX 13.3.1

@thehesiod thehesiod added feature-request This issue requests a feature. needs-triage This issue or PR still needs to be triaged. labels Apr 25, 2023
@thehesiod
Copy link
Author

added impl in #2914

@tim-finnigan
Copy link
Contributor

Hi @thehesiod thanks for creating the feature request. Here is boto3 documentation on waiters for reference: https://boto3.amazonaws.com/v1/documentation/api/latest/guide/clients.html#waiters. As mentioned there:

Waiters use a client’s service operations to poll the status of an AWS resource and suspend execution until the AWS resource reaches the state that the waiter is polling for or a failure occurs while polling

Since waiters are designed to only poll a resource's status and suspend execution, I don't think the change you proposed could be considered.

Could you expand a bit more on your use case and what you're trying to do? You mentioned using Athena, but there are currently no waiters available for that service: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/athena.html. (Although there is an open feature request to add waiters for Athena here: aws/aws-sdk#80)

@tim-finnigan tim-finnigan added response-requested Waiting on additional info and feedback. waiter and removed needs-triage This issue or PR still needs to be triaged. labels Apr 26, 2023
@thehesiod
Copy link
Author

here's the code I'm using:

_athena_waiter_config = {
    "version": 2,
    "waiters": {
        "QueryCompleted": {
            "description": "Wait until query completes",
            "operation": "GetQueryExecution",
            "delay": 1,
            "maxAttempts": 60 * 10,
            "acceptors": [
                {
                    "state": "failure",
                    "matcher": "path",
                    "expected": "FAILED",
                    "argument": "QueryExecution.Status.State",
                },
                {
                    "state": "failure",
                    "expected": "CANCELLED",
                    "matcher": "pathAny",
                    "argument": "QueryExecution.Status.State"
                },
                {
                    "state": "success",
                    "matcher": "path",
                    "expected": "SUCCEEDED",
                    "argument": "QueryExecution.Status.State",
                }
            ]
        }
    }
}


        start_response = await athena_client.start_query_execution(**query)

        query_id = start_response['QueryExecutionId']
        waiter = aioboto_waiter.create_waiter_with_client(
            'QueryCompleted',
            aioboto_waiter.WaiterModel(_athena_waiter_config),
            athena_client
        )

        try:
            get_response = await waiter.wait(QueryExecutionId=query_id)
        except WaiterError as e:
            if str(e) == 'Waiter QueryCompleted failed: Max attempts exceeded':
                raise AthenaQueryTimeoutError(query) from e

            raise AthenaQueryError(query) from e

        stats = get_response['QueryExecution']['Statistics'] or dict()
        span.set_tags(stats)
        span.set_tags({
            'request_id': get_response['ResponseMetadata']['RequestId'],
        })
        logger.info({
            "message": f"Executed query",
            "query_id": query_id,
            "start_execution_request_id": start_response['ResponseMetadata']['RequestId'],
            "get_query_execution_request_id": get_response['ResponseMetadata']['RequestId'],
            **stats
        })

        s3_output = None
        if return_output:
            output_s3_location = get_response['QueryExecution']['ResultConfiguration']['OutputLocation']

I start the query execution, the wait for it to finish. After it finishes I want to get the statistics to send to datadog, and potentially retrieve other items from the response like the output location. I don't want to have to call GetQueryExecution again as the waiter already has the last response.

@thehesiod
Copy link
Author

let me know if you'd like for me to add that waiter to my PR

@thehesiod
Copy link
Author

I don't see how my proposal conflicts with the documentation, it could be amended to state that it returns the last response received from the waiter. I suppose another option could be to have an emit for an event of post waiter response or something. But this seems much simpler.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. label Apr 26, 2023
@tim-finnigan
Copy link
Contributor

Thanks for following up and sharing that info. Waiter definitions are shared across SDKs so they need to be implemented by service teams rather that added directly to an individual SDK. I can check in with the Athena team for an update on the request to add waiters. But it sounds like your use case is specific to querying Athena statistics. Waiters are just intended to suspend execution and it would be unexpected to start returning a response for all waiters.

@thehesiod
Copy link
Author

the behavior stays the say, just that it returns the last response (if any). If you see the implementation there is no impact to service teams, it's a higher-level concept. I still don't understand the friction.

@tim-finnigan
Copy link
Contributor

Thanks for following up — just to clarify, I meant that the waiter definitions themselves are implemented by service teams and used across SDKs so we can't accept PRs for those. But I brought this issue up for discussion with the team, and the consensus was that what you're proposing warrants some further investigation. Specifically, we will want to look into how waiters in other AWS SDKs behave and if anything is returned in those processes.

It makes sense to avoid another method call if the waiter already has the response you need. One idea that was floated in our team discussion was that maybe a list of responses from throughout the polling calls could be made accessible.

This actually relates somewhat to another feature request we just received: #2923. The suggestion there is to log statuses during the execution of waiters. I think that is different enough to be tracked as a separate request but there may be some overlap between these use cases.

@tim-finnigan tim-finnigan removed their assignment Apr 28, 2023
@tim-finnigan tim-finnigan added the p2 This is a standard priority issue label Apr 28, 2023
@thehesiod
Copy link
Author

ahh ok :] I'm not sure returning all the responses is a good idea, you could theoretically have a lot in there if you're checking frequently and for a long time.

@thehesiod
Copy link
Author

FWIW I went ahead and implemented this in aiobotocore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue requests a feature. p2 This is a standard priority issue waiter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants