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

Metadata: add a function to access the stream map #71

Merged

Conversation

Jmgr
Copy link
Contributor

@Jmgr Jmgr commented Aug 24, 2020

This PR adds a function to access the stream map when fetching metadata. The current API only allows you to get a SteamInfo using a stream name.

@tylertreat
Copy link
Member

I think we ought to return a copy of the map rather than the actual underlying one? Just to prevent the ability to manipulate it from outside of Metadata.

@Jmgr
Copy link
Contributor Author

Jmgr commented Aug 25, 2020

Hm yes that would be more coherent with the other functions. I'll make that change.

Shouldn't StreamInfo.Partitions() also return a copy in that case?

Jonathan Mercier-Ganady added 2 commits August 25, 2020 10:22
@Jmgr
Copy link
Contributor Author

Jmgr commented Aug 25, 2020

I have made the change in aee6d4c.

I have also committed a code simplification in 8f9c1e0. Performance should be the same, so it's only a cosmetic change. What do you think?

@Jmgr
Copy link
Contributor Author

Jmgr commented Aug 25, 2020

Also, shouldn't those changes be made in the v2 module?

@tylertreat
Copy link
Member

Apologies for the delay on the response, was out all week.

Yeah, we should include the changes in the v2 module.

@tylertreat tylertreat linked an issue Aug 31, 2020 that may be closed by this pull request
@Jmgr
Copy link
Contributor Author

Jmgr commented Sep 1, 2020

Yeah, we should include the changes in the v2 module.

I have applied the changes to v2 metadata.go file and removed new function from v1.

@tylertreat
Copy link
Member

I have applied the changes to v2 metadata.go file and removed new function from v1.

Since the change is backwards compatible, it's OK to include in v1 if you are reliant upon v1 of the module. I think larger API changes will only be applied to v2 going forward but I think it's fine to backport smaller changes to v1.

@Jmgr
Copy link
Contributor Author

Jmgr commented Sep 1, 2020

Since the change is backwards compatible, it's OK to include in v1 if you are reliant upon v1 of the module. I think larger API changes will only be applied to v2 going forward but I think it's fine to backport smaller changes to v1.

Alright. I have reverted the function removal from v1.

@tylertreat tylertreat merged commit 38f14d2 into liftbridge-io:master Sep 1, 2020
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.

Metadata: how to list all streams?
2 participants