Feature/propagate context in ThreadLocal

faolivera commented 7 years ago

Changes made in order to propagate the Context using ThreadLocal Variables.

musiKk commented 7 years ago

Hi Facundo, I really appreciate the effort you put into this.

Now, the issue I see with this is that it is a breaking change that will create some amount of work to fix. I haven't migrated one of our projects so I don't have a complete understanding of the impact yet.

Do you reckon the feature can be added in a compatible way?
The code for the thread local context propagation is from another project whose README states that this has performance implications (whatever that means exactly) and that passing state explicitly instead of via thread locals is easier to reason about.

faolivera commented 7 years ago

Hi Werner, Thank you for your feedback, I'll prepare some performance test in order to understand better if there is some performance penalty or not. We can continue talking about this PR after the performance tests results.

musiKk commented 7 years ago

Great! How about the other point? I started to migrate one of our services and quickly found out that this basically changes so much to the point that this feels like a different library. Do you think it might be possible to have both the version with explicit context passing as well as via thread locals and the user can choose which to use? Probably by setting the default-dispatcher or not. Even if we were to ultimately use only the thread locals (and I definitely like them) this would greatly improve the migration story for existing users.

faolivera commented 7 years ago

Yes, I will changed the PR to make it compatible with the current implementation, but only after seeing the test performance results so we know if it worth it to invest more work on this feature or not :). I think it is possible to make it compatible, also I will add an implementation for an ExecutionContext that propagate the Context so you can choose if use the Dispatcher or just to use the ExecutionContext wherever the library user consider it necessary.

musiKk commented 7 years ago

Awesome. Because I like the idea.

faolivera commented 7 years ago

I created this project for running the performance test for this PR: https://github.com/faolivera/hutmann-performance-test it is possible to download it and run it.

Some Results

first run

Report for default Dispatcher Report for custom Dispatcher

Request Time	Default Dispatcher	Custom Dispatcher
<100 ms	9.95%	7.15%
>100ms < 200ms	10.4%	10.3%
>200ms < 300ms	9.55%	11.05%
>300ms < 400ms	7.25%	8.94%

second run

Report for default Dispatcher Report for custom Dispatcher

Request Time	Default Dispatcher	Custom Dispatcher
<100 ms	14.65%	8.75%
>100ms < 200ms	10.95%	8.19%
>200ms < 300ms	8.69%	9.75%
>300ms < 400ms	5.05%	7.74%

third run

Report for default Dispatcher Report for custom Dispatcher

Request Time	Default Dispatcher	Custom Dispatcher
<100 ms	19.65%	12.08%
>100ms < 200ms	11.05%	6.55%
>200ms < 300ms	11.55%	7.4%
>300ms < 400ms	2.75%	6.8%

musiKk commented 7 years ago

Apologies for the long waiting time.

I think the values are good enough that it would warrant inclusion in the library. Then again, I'd like to defer the decision to you as the work would fall on you. If the version with the custom dispatcher is just a new feature and the library would behave no different for existing users, I basically don't care that much about the performance of the new feature.

faolivera commented 7 years ago

Hi! Thank you for taking the time to review the PR. I made rollback some changes and add new ones in order to maintain the compatibility with the existing version. You can update the library version in your project and don't even notice the changes. Please let me know if there is something else that I can do.

musiKk commented 7 years ago

:+1:

faolivera commented 7 years ago

👍

zalando-stups / hutmann