Skip to content

Conversation

@dcantah
Copy link
Contributor

@dcantah dcantah commented Aug 19, 2021

To avoid a breaking change on GetSupportedFeatures introduce a new
GetCachedSupportedFeatures method. This method does the feature check
and version parsing once and then assigns a global with the information.

This can be used to optimize for situations where many uses of the
hcn.IsXSupported methods are going to be used (kube-proxy for example).
Previously they would call GetSupportedFeatures and end up re-querying and
revalidating the feature ranges every time.

Signed-off-by: Daniel Canter [email protected]

To avoid a breaking change on GetSupportedFeatures introduce a new
GetCachedSupportedFeatures method. This method does the feature check
and version parsing once and then assigns a global with the information.
This can be used to optimize for situations where many uses of the
hcn.IsXSupported methods are going to be used (kube-proxy for example).

Signed-off-by: Daniel Canter <[email protected]>
@dcantah dcantah requested a review from a team as a code owner August 19, 2021 03:15
@dcantah
Copy link
Contributor Author

dcantah commented Aug 19, 2021

@kevpar @msscotb Feedback addressed, ptal when you get another chance. Thanks!

- Change versionErr -> featuresErr
- Move work inside of the sync.Once out to its own function so we
can use standard return err error handling and simply assign the
output in GetCachedSupportedFeatures
- Mark GetSupportedFeatures as deprecated.

Signed-off-by: Daniel Canter <[email protected]>
@dcantah dcantah force-pushed the new-hcn-features-call branch from 5cd55e5 to f78d0ff Compare August 19, 2021 18:01
return supportedFeatures, featuresErr
}

// Deprecated: Use GetCachedSupportedFeatures instead.
Copy link
Member

Choose a reason for hiding this comment

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

I think the main comment needs to be first or it will be flagged as not following function comment guidelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants