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

Adapter aliases - change coreBidderNames to return aliases #3027

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

gargcreation1992
Copy link
Contributor

This PR makes changes to CoreBidderNames function to also return alias bidder names. Once we complete this feature, current alias bidder names from CoreBidderNames function will be removed and we will be relying on newly introduced aliasBidderNames.

@SyntaxNode
Copy link
Contributor

Why is it necessary to separate the concept of "core bidders" from aliases? Today, all aliases are already part of the "core bidders" list, except for those defined in the default request, which will be deprecated when this approach is complete.

@gargcreation1992
Copy link
Contributor Author

Why is it necessary to separate the concept of "core bidders" from aliases? Today, all aliases are already part of the "core bidders" list, except for those defined in the default request, which will be deprecated when this approach is complete.

The eventual objective of this strategy is to establish alias information exclusively through the bidder-info YAML configuration. At present, the CoreBidderNames function encompasses both the concrete bidder names and their aliases. Upon completion of this approach, we will transition the current aliases to adhere to this new approach. The hardcoded alias bidders sourced from CoreBidderNames will be eliminated, and instead, bidderName will be generated from the YAML, utilising the filename as their corresponding BidderName.
Please let me know if I am missing something here.

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Added comments with the refactoring proposal which I hope will make code a little cleaner, save memory and eliminate unnecessary evaluations. Let me know what do you think.

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Thank you for the changes. Looks good to me.

@hhhjort hhhjort self-requested a review August 17, 2023 17:53
@hhhjort hhhjort self-assigned this Aug 17, 2023
@@ -292,7 +306,7 @@ const (

// CoreBidderNames returns a slice of all core bidders.
func CoreBidderNames() []BidderName {
return []BidderName{
return append([]BidderName{
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to use package variable here. CoreBidderNames() gets called a lot. This was fine when we were just returning a static value, at worst we were doing an extra reference to get the slice. But now we are executing an append function, which is going to reallocate a new chunk of memory on every call, in addition to copying all the values to this new memory location. We should do this just once, and then always return that combined slice on every call. Probably by modifying SetAliasBidderName() to construct the full slice that this function will return.

Copy link
Contributor Author

@gargcreation1992 gargcreation1992 Aug 18, 2023

Choose a reason for hiding this comment

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

fixed here - ec44c52

Copy link
Collaborator

@hhhjort hhhjort 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