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

KIP-42 Add producer and consumer interceptors #1730

Merged
merged 6 commits into from Jul 15, 2020

Conversation

d1egoaz
Copy link
Contributor

@d1egoaz d1egoaz commented Jun 22, 2020

KIP-42 Add producer and consumer interceptors

This PR includes:
Producer: OnSend but it doesn't implement onAcknowledgement
Consumer: OnConsume but it doesn't implement onCommit

I tried on purpose to not follow the Java design too closely. This PR tries to follow Go idioms to the degree possible.
I didn't add an onClose method on the interface, Go has other alternatives for this if needed.

for:

Since there may be multiple interceptors, the first interceptor will get a record from client passed as the 'record' parameter. The next interceptor in the list will get the record returned by the previous interceptor, and so on. Since interceptors are allowed to mutate records, interceptors may potentially get the record already modified by other interceptors. However, we will state in the javadoc that building a pipeline of mutable interceptors that depend on the output of the previous interceptors is discouraged, because of potential side-effects caused by interceptors potentially failing to mutate the record and throwing and exception. If one of the interceptors in the list throws an exception from onSend(), the exception is caught, logged,and the next interceptor is called with the record returned by the last successful interceptor in the list, or otherwise the client.

specifically the section:

If one of the interceptors in the list throws an exception from onSend(), the exception is caught, logged,and the next interceptor is called with the record returned by the last successful interceptor in the list, or otherwise the client.

My implementation does the following, as I'm passing the value by reference:
If one of the interceptors in the list throws an exception from onSend(), the exception is caught, logged,and the next interceptor is called with the record returned by the last successful interceptor in the list.

The interceptor method needs to make sure to only modify the object when it's safe to do so, otherwise it can modify the object and then fail, which will make the next interceptor to use a modified value from maybe a non successful interceptor.
For some reason, Sarama ProducerMessage contains a channel, which makes hard to clone this object.

Another alternative will be to make a copy of the body, headers, topic, partition data, and if the interceptor call fails, revert the message to this original data, but I'm not sure if it worth the effort, as anyone working with go know that if you modify an object passed as a reference, it'll modify the source object.

Closes #1710

Copy link

@psycotica0-shopify psycotica0-shopify left a comment

Choose a reason for hiding this comment

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

Code looks alright to me.

interceptors.go Outdated Show resolved Hide resolved
@d1egoaz d1egoaz mentioned this pull request Jun 25, 2020
This PR includes:
Producer: `onSend` but it doesn't implement `onAcknowledgement`
Consumer: `onConsume` but it doesn't implement `onCommit`

I'm not sure if I need to add the `onClose` method. Maybe in another
iteration ¯\_(ツ)_/¯
Copy link
Contributor

@bai bai left a comment

Choose a reason for hiding this comment

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

:shipit:

@dnwe
Copy link
Collaborator

dnwe commented Jun 30, 2020

@d1egoaz approved, changes LGTM.

It would be good to have something new in the examples/ tree that makes use of this. Would you be able to look into that under a follow-on PR?

@d1egoaz
Copy link
Contributor Author

d1egoaz commented Jul 15, 2020

@dnwe for sure, I'll add an example soon

@d1egoaz d1egoaz merged commit ec4f2e8 into IBM:master Jul 15, 2020
@d1egoaz d1egoaz deleted the diego_interceptors branch July 15, 2020 15:13
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.

KIP-42: add producer and consumer interceptors
4 participants