HIVE-2849: Migrate AWS SDK to v2 #2695
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@2uasimojo: This pull request references HIVE-2849 which is a valid jira issue. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
2uasimojo
force-pushed
the
HIVE-2849/aws-sdk-v2
branch
from
9d3441a to
011067d
Compare
| } | ||
|
|
||
| func (c *awsClient) WaitUntilVpcPeeringConnectionDeleted(input *ec2.DescribeVpcPeeringConnectionsInput) error { | ||
| metricAWSAPICalls.WithLabelValues("WaitUntilVpcPeeringConnectionDeleted").Inc() |
There was a problem hiding this comment.
@jstuever @suhanime request 👀 here.
The point of the metric is so we can track how many times we call a given API. This name (WaitUntilVpcPeeringConnectionDeleted) is from the v1 SDK. I've kept the name for our shim. Question is, is it cool to keep it as the metric label, or should we change it to something else -- like VpcPeeringConnectionDeleted__Wait?
There was a problem hiding this comment.
A consideration: changing the label would break continuity (start a new time series rather than continuing to add to the existing one) when upgrading hive across the boundary of this commit...
There was a problem hiding this comment.
I was thinking of other labels and thought about grouping it according to their core function first - aka, Authorize_* , Describe_* , Revoke_* - and by that logic - Wait_* . The one for wait might need rework to make it make sense on first glance though, or maybe WaitUntil_*?
Also, label name changes are still temporary, and as long as there's no query configured on a dashboard somewhere, it isn't a breaking change. There will be a new time series every time hive is upgraded anyways.
There was a problem hiding this comment.
If the v1 and v2 SDK funcs have the same name, I see no reason to change the label. If there's some convincing argument for doing so, we can talk about it under a separate card/effort.
The question is what to do about these new cases where the v1 SDK func name does not exist in v2 and we can't use the exact name of the new func -- in this case because it's a generically-named method on an "object" of a specific "class". Your suggestion to use WaitUntil_* actually makes the decision pretty crisp for me: In this case it would mean we're changing WaitUntilVpcPeeringConnectionDeleted to WaitUntil_VpcPeeringConnectionDeleted, which seems not at all worth doing. And the delta between that and Wait_* or *_Wait is not significant. So, unless there's some strong reason to do otherwise, I think I'm going to keep the existing labels wherever feasible.
Thanks for the look!
2uasimojo
force-pushed
the
HIVE-2849/aws-sdk-v2
branch
2 times, most recently
from
3749bc4 to
76572f3
Compare
2uasimojo
force-pushed
the
HIVE-2849/aws-sdk-v2
branch
from
76572f3 to
c7af75f
Compare
No description provided.