Change gen_fsm to gen_server behaviour #20

Merged
merged 2 commits into from Dec 18, 2012

Conversation

Projects
None yet
3 participants
@ddosia
Contributor

ddosia commented Dec 1, 2012

Drop gen_fsm in favor of gen_server. 1/4 code from poolboy.erl is gone.
Is it worth to be merged into master?

@Vagabond

This comment has been minimized.

Show comment Hide comment
@Vagabond

Vagabond Dec 2, 2012

Collaborator

This even seems to pass the quickcheck tests, which is nice. I'll try to do some more review soon.

Collaborator

Vagabond commented Dec 2, 2012

This even seems to pass the quickcheck tests, which is nice. I'll try to do some more review soon.

@devinus

This comment has been minimized.

Show comment Hide comment
@devinus

devinus Dec 3, 2012

Owner

I'll try to do some benchmarking this week.

Owner

devinus commented Dec 3, 2012

I'll try to do some benchmarking this week.

@Vagabond

This comment has been minimized.

Show comment Hide comment
@Vagabond

Vagabond Dec 3, 2012

Collaborator

Actually, longer EQC runs are exposing some issues, I think there might be a bug lurking.

Collaborator

Vagabond commented Dec 3, 2012

Actually, longer EQC runs are exposing some issues, I think there might be a bug lurking.

@ddosia

This comment has been minimized.

Show comment Hide comment
@ddosia

ddosia Dec 7, 2012

Contributor

Any way to reproduce this tests?
Despite this issue, how about the idea? Currently i trying to add new functionality to poolboy, and it is much easier in gen_server version then in original.

Contributor

ddosia commented Dec 7, 2012

Any way to reproduce this tests?
Despite this issue, how about the idea? Currently i trying to add new functionality to poolboy, and it is much easier in gen_server version then in original.

@devinus

This comment has been minimized.

Show comment Hide comment
@devinus

devinus Dec 7, 2012

Owner

The idea is great. As long as performance doesn't suffer, who can argue against simpler code?

Owner

devinus commented Dec 7, 2012

The idea is great. As long as performance doesn't suffer, who can argue against simpler code?

@Vagabond

This comment has been minimized.

Show comment Hide comment
@Vagabond

Vagabond Dec 7, 2012

Collaborator

Still trying to decide if the quickcheck problems are just line noise, sorry.

Collaborator

Vagabond commented Dec 7, 2012

Still trying to decide if the quickcheck problems are just line noise, sorry.

@Vagabond

This comment has been minimized.

Show comment Hide comment
@Vagabond

Vagabond Dec 7, 2012

Collaborator

But I think we should pursue this branch, just want to make sure this change is solid first.

Collaborator

Vagabond commented Dec 7, 2012

But I think we should pursue this branch, just want to make sure this change is solid first.

@Vagabond

This comment has been minimized.

Show comment Hide comment
@Vagabond

Vagabond Dec 7, 2012

Collaborator

It looks like the EQC test had a timeout in it that was too short and it'd randomly timeout trying to do a checkout. I think we can ignore the failure and move forward with this.

Collaborator

Vagabond commented Dec 7, 2012

It looks like the EQC test had a timeout in it that was too short and it'd randomly timeout trying to do a checkout. I think we can ignore the failure and move forward with this.

@ghost ghost assigned devinus Dec 7, 2012

@devinus

This comment has been minimized.

Show comment Hide comment
@devinus

devinus Dec 17, 2012

Owner

@Vagabond I don't think there's any performance difference. As long as there are no regressions I'm going to merge this. Any last thoughts?

Owner

devinus commented Dec 17, 2012

@Vagabond I don't think there's any performance difference. As long as there are no regressions I'm going to merge this. Any last thoughts?

@Vagabond

This comment has been minimized.

Show comment Hide comment
@Vagabond

Vagabond Dec 18, 2012

Collaborator

+1 from me.

Collaborator

Vagabond commented Dec 18, 2012

+1 from me.

devinus pushed a commit that referenced this pull request Dec 18, 2012

Devin Torres
Merge pull request #20 from ddosia/dch-gen_server
Change gen_fsm to gen_server behaviour

@devinus devinus merged commit 6ddc61a into devinus:master Dec 18, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment