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

fixed IsAlive calculation in FbConnectionPoolManager.Pool.IsAlive #75

Closed
wants to merge 3 commits into
base: master
from

Conversation

2 participants
@realic

realic commented Dec 15, 2017

Hey Jiri
I found an error in version 5.12.0. I believe it was introduced when fixing DNET-787.

Please advise if the unittest is not implementet in a way you like.

@cincuranet

This comment has been minimized.

Show comment
Hide comment
@cincuranet

cincuranet Dec 15, 2017

Owner

Fix the PR to not have unnecessary changes, please. Also the test and method can be extracted so the whole stuff does not have to be internal.

Owner

cincuranet commented Dec 15, 2017

Fix the PR to not have unnecessary changes, please. Also the test and method can be extracted so the whole stuff does not have to be internal.

@realic

This comment has been minimized.

Show comment
Hide comment
@realic

realic Dec 18, 2017

Do you want me to extract the IsAlive and GetTicks to a seperate publice testable class?

realic commented Dec 18, 2017

Do you want me to extract the IsAlive and GetTicks to a seperate publice testable class?

@cincuranet

This comment has been minimized.

Show comment
Hide comment
@cincuranet

cincuranet Dec 18, 2017

Owner

Yes. I would put into Common something like ConnectionLifetimeHelper (internal), with IsAlive method. And I would test this. The GetTicks is not needed.

Owner

cincuranet commented Dec 18, 2017

Yes. I would put into Common something like ConnectionLifetimeHelper (internal), with IsAlive method. And I would test this. The GetTicks is not needed.

@realic

This comment has been minimized.

Show comment
Hide comment
@realic

realic Dec 20, 2017

The requested changes have been implemented, if you haven't already received a notification about it.

realic commented Dec 20, 2017

The requested changes have been implemented, if you haven't already received a notification about it.

@cincuranet

This comment has been minimized.

Show comment
Hide comment
@cincuranet

cincuranet Dec 21, 2017

Owner

Merged by 138a347.

Owner

cincuranet commented Dec 21, 2017

Merged by 138a347.

@cincuranet cincuranet closed this Dec 21, 2017

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