Skip to content

Conversation

@solmonk
Copy link
Contributor

@solmonk solmonk commented Nov 10, 2023

What type of PR is this?

enhancement

Which issue does this PR fix:

What does this PR do / Why do we need it:

  • Add caching to ListTagsForResource calls, we may expand this to other calls as well
  • Optimize list operation of target group
    • fetch tags at once using GetResources() api
    • Do not make unnecessary get() calls, as tgSummary is sufficient in most cases
  • Add ProtocolVersion on TG tags, this saves multiple GetTargetGroup() calls for TG cleanup.
  • Optimize find operation of target group
    • Utilizes GetResources() api to filter out target group

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

Testing done on this change:

Automation added to e2e:

Will this PR introduce any new dependencies?:

Will this break upgrades or downgrades. Has updating a running cluster been tested?:

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


ResourceTypeTargetGroup ResourceType = resourceTypePrefix + "targetgroup"

maxArnsPerGetResourcesApi = 100
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you add link to the limit please

if err != nil {
return model.TargetGroupStatus{},
fmt.Errorf("Failed UpdateTargetGroup %s due to %s", aws.StringValue(latticeTgSummary.Id), err)
fmt.Errorf("Failed UpdateTargetGroup %s due to %s", aws.StringValue(latticeTg.Id), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: %w for err

Comment on lines 288 to 289
// Can get only one arn through the above search criteria.
arn := arns[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

lets throw or log error when assumption does not work

if match {
switch status {
case vpclattice.TargetGroupStatusCreateInProgress, vpclattice.TargetGroupStatusDeleteInProgress:
return nil, errors.New(LATTICE_RETRY)
Copy link
Contributor

Choose a reason for hiding this comment

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

retrun fmt.Errorf("%w, delete in progress id=%d", runtime.RetryErr)

@solmonk solmonk merged commit 6e7ea95 into aws:main Nov 14, 2023
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