Skip to content

refactor #13

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

Merged
merged 6 commits into from
Mar 8, 2020
Merged

refactor #13

merged 6 commits into from
Mar 8, 2020

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Mar 6, 2020

motivation: safer concurrency, better tests

changes:

  • make http client thread safe
  • add concurrency tests
  • refactor test
  • make mock server more ribust

@tomerd tomerd requested a review from fabianfett March 6, 2020 20:51
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

All in all this makes further work much easier. Thanks.
I've added some comments. Most of them are probably failures in understanding at my side.

tomerd added a commit that referenced this pull request Mar 7, 2020
motivation: make lambda execution sequence easier to reason about

changes:
* add `peekFailure` extension to `EventLoopFuture` to handle error without side effects
* update process chain to make use of `peekFailure`
tomerd added a commit that referenced this pull request Mar 7, 2020
motivation: make lambda execution sequence easier to reason about

changes:
* add `peekFailure` extension to `EventLoopFuture` to handle error without side effects
* update process chain to make use of `peekFailure`
tomerd added a commit that referenced this pull request Mar 7, 2020
motivation: make lambda execution sequence easier to reason about

changes:
* add `peekFailure` extension to `EventLoopFuture` to handle error without side effects
* update process chain to make use of `peekFailure`
tomerd added a commit that referenced this pull request Mar 7, 2020
motivation: make lambda execution sequence easier to reason about

changes:
* add `peekFailure` extension to `EventLoopFuture` to handle error without side effects
* update process chain to make use of `peekFailure`
tomerd added a commit that referenced this pull request Mar 7, 2020
motivation: make lambda execution sequence easier to reason about

changes:
* add `peekFailure` extension to `EventLoopFuture` to handle error without side effects
* update process chain to make use of `peekFailure`
@tomerd tomerd force-pushed the refactor branch 2 times, most recently from 95383be to 8fe2986 Compare March 7, 2020 02:24
tomerd added 3 commits March 6, 2020 18:48
motivation: safer concurrency, better tests

changes:
* make http client thread safe
* add concurrency tests
* refactor test
* make mock server more ribust
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Wow. Looking way better than before. Most comments are about a potential memory leak that I can see and otherwise just ensuring state.

@tomerd tomerd mentioned this pull request Mar 7, 2020
@tomerd
Copy link
Contributor Author

tomerd commented Mar 8, 2020

@fabianfett I think this is ready for final review/merge

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

I'm very happy with this!

@fabianfett fabianfett mentioned this pull request Mar 8, 2020
@tomerd tomerd merged commit 08f75f5 into master Mar 8, 2020
@tomerd tomerd deleted the refactor branch March 10, 2020 18:19
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.

3 participants