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

Compression promises #50

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@cjlarose
Contributor

cjlarose commented Apr 18, 2016

There were a number of branches in the Client code that were trying to deal with whether or not the compression technique was synchronous or asynchronous. One of the greatest benefits of using Promises, though, is that they can unify the interactions between sync and async code.

I modified the synchronous compression functions to return Promises, just like their asynchronous counterparts. This allowed for a number of upstream simplifications of the Client code.

I split the changes up into separate commits to make reviewing easier, but I can squash if it is preferred.

@oleksiyk

This comment has been minimized.

Show comment
Hide comment
@oleksiyk

oleksiyk Apr 18, 2016

Owner

This looks great, thanks!

Just that I wanted to remove sync compression in version 3.0. Sync/Async compression was created for a performance comparison test which never happened unfortunately but in most cases async code is preferred in Node.

Owner

oleksiyk commented Apr 18, 2016

This looks great, thanks!

Just that I wanted to remove sync compression in version 3.0. Sync/Async compression was created for a performance comparison test which never happened unfortunately but in most cases async code is preferred in Node.

@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Apr 18, 2016

Contributor

That's great! The only changes that would have to be made in the Client, then, for removing the sync compression methods would be to remove the two places where there's a branch for asyncCompression.

https://github.com/oleksiyk/kafka/pull/50/files#diff-50cfa59973c04321b5da0c6da0fdf4feR643

and

https://github.com/oleksiyk/kafka/pull/50/files#diff-50cfa59973c04321b5da0c6da0fdf4feR634

Contributor

cjlarose commented Apr 18, 2016

That's great! The only changes that would have to be made in the Client, then, for removing the sync compression methods would be to remove the two places where there's a branch for asyncCompression.

https://github.com/oleksiyk/kafka/pull/50/files#diff-50cfa59973c04321b5da0c6da0fdf4feR643

and

https://github.com/oleksiyk/kafka/pull/50/files#diff-50cfa59973c04321b5da0c6da0fdf4feR634

@oleksiyk

This comment has been minimized.

Show comment
Hide comment
@oleksiyk

oleksiyk Apr 19, 2016

Owner

Honestly, I think I would prefer to keep existing code. It might not look great but it does less asynchronous mappings such as: https://github.com/cjlarose/kafka/blob/compression-promises/lib/client.js#L266-L267

Existing code is bad because its using huge side effect by modifying variable from upper scope: https://github.com/oleksiyk/kafka/blob/master/lib/client.js#L248 The downside of proposed changes is duplicate async iteration(s) which will render useless context switching.

Owner

oleksiyk commented Apr 19, 2016

Honestly, I think I would prefer to keep existing code. It might not look great but it does less asynchronous mappings such as: https://github.com/cjlarose/kafka/blob/compression-promises/lib/client.js#L266-L267

Existing code is bad because its using huge side effect by modifying variable from upper scope: https://github.com/oleksiyk/kafka/blob/master/lib/client.js#L248 The downside of proposed changes is duplicate async iteration(s) which will render useless context switching.

@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Apr 20, 2016

Contributor

If you're concerned about the performance implications of the changes, I agree with you that the proposed changes to Client#produceRequest would have a negative effect. It would create a bunch more intermediate Promise objects that aren't being created now and would push more events into future iterations of the event loop.

However, I'd encourage you to take a look at the changes made to Client#fetchRequest, because here, I think, the performance characteristics are improved.

In the old code, in the case where the message sets are compressed, each time a message set is decompressed, a new intermediate Array is generated with a call to Array.prototype.concat. This is true for both the async compression case as well as the synchronous compression case.

The new code instead collects all of the results at the end and generates a single new array with the call to _.flatten. It introduces no additional Promise objects on top of what was being created beforehand, and doesn't seem to introduce anything that would have a performance penalty.

Contributor

cjlarose commented Apr 20, 2016

If you're concerned about the performance implications of the changes, I agree with you that the proposed changes to Client#produceRequest would have a negative effect. It would create a bunch more intermediate Promise objects that aren't being created now and would push more events into future iterations of the event loop.

However, I'd encourage you to take a look at the changes made to Client#fetchRequest, because here, I think, the performance characteristics are improved.

In the old code, in the case where the message sets are compressed, each time a message set is decompressed, a new intermediate Array is generated with a call to Array.prototype.concat. This is true for both the async compression case as well as the synchronous compression case.

The new code instead collects all of the results at the end and generates a single new array with the call to _.flatten. It introduces no additional Promise objects on top of what was being created beforehand, and doesn't seem to introduce anything that would have a performance penalty.

@oleksiyk

This comment has been minimized.

Show comment
Hide comment
@oleksiyk

oleksiyk Apr 20, 2016

Owner

You are right, Client#fetchRequest looks much better.
How about we merge both ways then? Keep old produceRequest code and use new fetchRequest?
I can manually merge your PR and then do all required changes.

Owner

oleksiyk commented Apr 20, 2016

You are right, Client#fetchRequest looks much better.
How about we merge both ways then? Keep old produceRequest code and use new fetchRequest?
I can manually merge your PR and then do all required changes.

@cjlarose

This comment has been minimized.

Show comment
Hide comment
@cjlarose

cjlarose Apr 20, 2016

Contributor

Sounds good to me!

Contributor

cjlarose commented Apr 20, 2016

Sounds good to me!

@oleksiyk

This comment has been minimized.

Show comment
Hide comment
@oleksiyk

oleksiyk Apr 22, 2016

Owner

Manually merged in master. Thanks a lot!

Owner

oleksiyk commented Apr 22, 2016

Manually merged in master. Thanks a lot!

@oleksiyk oleksiyk closed this Apr 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment