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

Custom cancelable implementation replaced with context #378

Merged
merged 4 commits into from Sep 19, 2018

Conversation

Projects
None yet
5 participants
@soffokl
Copy link
Member

commented Sep 18, 2018

No description provided.

@soffokl soffokl added the enhancement label Sep 18, 2018

@soffokl soffokl self-assigned this Sep 18, 2018

@soffokl soffokl requested review from tadovas, Waldz, zolia and vkuznecovas Sep 18, 2018

@tadovas
Copy link
Member

left a comment

I am not sure if this adds much improvement or cleaner code. @Waldz @zolia @vkuznecovas your opinions?

@vkuznecovas

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

Mixed feelings here.

On one hand, we get rid of the custom implementation of cancellable and resolve to using a standard tool for that.

On the other hand, I feel like we end up passing a whole lot of contexts around for little benefit.

@zolia

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

It is good to see that large amount of code was reduced, I would be inclined to accept, just that context spreads around too much, even when it is not needed and its depth might be uncontrollable.
Also, this context does not pass any value, just ability to cancel. Maybe we can abstract this cancellable functionality and reduce code this way? I see that it reduces codebase for 250 line or so, maybe we can accept this change with less spread of context?

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

OK, clear.
I'll do it with a smaller spreading.
And will ask for another review.
Thank you all for the feedback.

@soffokl soffokl force-pushed the cancelable-context branch 2 times, most recently from b10e7e6 to 2722928 Sep 18, 2018

@@ -47,16 +47,16 @@ var (
)

type connectionManager struct {
ctx context.Context

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 18, 2018

Member

Should be in second group of attributes

soffokl added some commits Sep 18, 2018

@soffokl soffokl force-pushed the cancelable-context branch from 2e88e82 to 05c62ab Sep 19, 2018

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2018

@Waldz @vkuznecovas @zolia @tadovas please take a look on this once again. We, probably, should decide do we need it or not.

@Waldz

Waldz approved these changes Sep 19, 2018

@tadovas
Copy link
Member

left a comment

Well now this looks more like it now. Context isolated inside connection manager interface - will be easily refactored if we find another way. LGTM as long as tests pass :)

@zolia

zolia approved these changes Sep 19, 2018

@zolia zolia merged commit 1d49785 into master Sep 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@zolia zolia deleted the cancelable-context branch Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.