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

Group subscriber v2 terminate #410

Merged
merged 13 commits into from Jun 22, 2020
Merged

Group subscriber v2 terminate #410

merged 13 commits into from Jun 22, 2020

Conversation

k32
Copy link

@k32 k32 commented Jun 16, 2020

This PR introduces an optional brod_group_subscriber_v2 terminate callback.

@coveralls
Copy link

coveralls commented Jun 16, 2020

Pull Request Test Coverage Report for Build 1446

  • 22 of 26 (84.62%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 80.343%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/brod_topic_subscriber.erl 12 14 85.71%
src/brod_topic_subscriber_cb_fun.erl 3 5 60.0%
Totals Coverage Status
Change from base Build 1421: 0.1%
Covered Lines: 2248
Relevant Lines: 2798

💛 - Coveralls

@k32 k32 force-pushed the group_subscriber_v2_terminate branch from 8fe8cf7 to 2d769c9 Compare June 17, 2020 12:26
@k32 k32 requested a review from a team June 17, 2020 13:44
@@ -70,9 +70,13 @@
]).

%% Subscriber API
-export([ start_link_group_subscriber_v2/1
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's unfortunate that there's a _v2 in the name as the /1 arity makes it unambiguous. Maybe make the _v2 function a compat alias for start_link_group_subscriber/1, then drop the _v2 one when the old API is dropped?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if I follow you here: brod_group_subscriber_v2:start_link/1 is an existing API that was introduced together with brod_group_subsriber_v2 behavior, so renaming it won't be BWC. I didn't add a similar new API for the old brod_group_subscriber module, because I consider it problematic, and I hope to get it removed in the next major release (and rename brod_group_subscriber_v2 module into brod_group_subscriber).

@k32 k32 force-pushed the group_subscriber_v2_terminate branch from 86251a5 to c30e5fa Compare June 18, 2020 14:27
@k32 k32 merged commit a468e53 into master Jun 22, 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.

None yet

3 participants