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 an optional response callback to Producer #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bpot
Copy link
Owner

@bpot bpot commented Jul 10, 2014

Adds a callback that can be used to determine which messages failed to send and why.

#
# @return [Boolean]
def success?
produce_partition_response.error == Poseidon::NO_ERROR_CODE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could produce_partition_repsonse.error ever be nil? If so, would that mean there was no error?

Copy link
Owner Author

Choose a reason for hiding this comment

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

If should only ever be zero if there is no error.

@kerinin
Copy link

kerinin commented Jul 10, 2014

Seems cool. Might be worth documenting a use-case somewhere

end
end

# The messages we the produce request sent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo in this sentence.

@bpot
Copy link
Owner Author

bpot commented Jul 10, 2014

@kerinin true. I'll add it to the top-level Producer example

@bpot
Copy link
Owner Author

bpot commented Jul 10, 2014

@kerinin @atharrison There is also another route that we could go which is providing a future-like object which the user would iterate through once completed.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.13%) when pulling d29f069 on bp/response_callback into abbd3df on master.

@kerinin
Copy link

kerinin commented Jul 10, 2014

@bpot returning an iterator could get complicated with the retries - this seems like a more straightforward approach.

@atharrison
Copy link
Collaborator

Having a Callback seems more Ruby-esque, while having a Future that you block on reading the results from feels more 'concurrent'.

@bpot
Copy link
Owner Author

bpot commented Jul 10, 2014

Cool. Just realized this design didn't consider errors that occur before we send the request (like not being able to connect to a leader). I don't know if it makes sense to overload the ProduceResult for that case or pass a second exception? value to the block.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling c703749 on bp/response_callback into abbd3df on master.

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