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

Add getter for grpc fallback urls #47

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Conversation

Nomad-Free-Talent
Copy link
Contributor

No description provided.

@@ -120,6 +120,14 @@ impl CosmosBuilder {
self.grpc_fallback_urls.push(url.into().into());
}

/// gRPC fallback URLs
pub fn grpc_fallback_urls_ref(&self) -> Vec<&String> {
Copy link
Member

Choose a reason for hiding this comment

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

This approach forces an allocation of a Vec, most likely unnecessarily. I see a few alternative return types:

  • &Vec<Arc<String>>: most direct, loses no power versus this return type, but exposes internals of the library (the presence of the Arc). Given that we aren't provided API stability right now, I don't see a problem with that last part.
  • &[Arc<String>]: very similar to the above. I've actually never figured out whether it's more idiomatic to return a &Vec or &[] from a function. (For input, it's always better to take the slice to make the API more general.)
  • impl Iterator<&str>: best for hiding the internal API, doesn't add any cost, but slightly less flexible on the calling side

I slightly lean towards the first option. @psibi what do you think?

Copy link
Contributor Author

@Nomad-Free-Talent Nomad-Free-Talent Oct 22, 2024

Choose a reason for hiding this comment

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

In case we choose the first option, there's no need to have this additional method but change the visibility of the grpc_fallback_urls method.

pub(crate) fn grpc_fallback_urls(&self) -> &Vec<Arc<String>> {
&self.grpc_fallback_urls
}

What do you think @snoyberg ?

Copy link
Member

Choose a reason for hiding this comment

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

It's definitely a true statement. However, the advantage of keeping the field private is it gives more flexibility in the future to update the API. But given my other comments about not providing API compatibility, it's not strictly necessary. I kept it that way up until now because it was easy enough to provide the getter/setter approach. I'm not convinced it's worth it, so certainly open to changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case I'm gonna change the visibility of grpc_fallback_urls method and remove the current change.

@snoyberg snoyberg merged commit 133778b into main Oct 23, 2024
1 check passed
@snoyberg snoyberg deleted the add-getter-for-grpc-fallbacks branch October 23, 2024 00:42
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