Add worker_process_options for start and start_link purposes #75

Merged
merged 1 commit into from Nov 19, 2012

Conversation

Projects
None yet
2 participants
Contributor

norton commented Oct 10, 2012

Add worker_process_options for start and start_link purposes

Typically used to specify non-default, garbage collection options.

I would like to have your feedback for this patch. Is this (or something like it) patch of interest to you?

Have you received any feedback from ibrowse users regarding "long_gc" of ibrowse's worker processes?

Owner

cmullaparthi commented Oct 11, 2012

I can't remember receiving feedback about "long_gc" from anyone, but the patch seems useful. I have a couple of comments about this:

  • Instead of adding a new function, I would add spawn_worker_process/3 and spawn_link_worker_process/3. This makes it clear that the overall behaviour is the same, but additional options are being supplied.
  • Instead of having another argument to the try_routing_request, I would extract the options in the try_routing_request itself and then pass it on. Localises the change nicely.

Thank you.

Contributor

norton commented Oct 11, 2012

Can you write down the desired specs for spawn_worker_process/3 and spawn_link_worker_process/3 ?

thanks,

Owner

cmullaparthi commented Oct 12, 2012

Clever ploy on your part to make me think!

@SPEC spawn_worker_process(Url::string())
@SPEC spawn_worker_process(Host::string(), Port::integer())
@SPEC spawn_worker_process(Url::string(), Worker_process_options::list())
@SPEC spawn_worker_process(Host::string(), Port::integer(), Worker_process_options::list())

Same for the linked worker case.

Having written them down, I can see that the 2-arity clause is overloaded, and might cause confusion. I would still go for this over introducing a function with a new name in the API.

Contributor

norton commented Oct 12, 2012

Agh ... you saw through my clever ploy!

If you would permit a change in API, I'd prefer the following API.

@SPEC spawn_worker_process(Url::string() | {Host::string(), Port::integer()})
@SPEC spawn_worker_process(Url::string() | {Host::string(), Port::integer()}, Worker_process_options::list())

For backwards compatibility, the API could detect the old usage and then convert it to the new format.

Owner

cmullaparthi commented Oct 12, 2012

Yes, I can live with that. Just have to bump up the major version.

Contributor

norton commented Oct 15, 2012

Ok, great. I'll let you know after I rework the commit.

@norton norton Add worker_process_options for start and start_link purposes
Typically used to specify non-default, garbage collection options.
1a80940
Contributor

norton commented Oct 24, 2012

I reworked the commit. Please review when you have time.

@cmullaparthi cmullaparthi added a commit that referenced this pull request Nov 19, 2012

@cmullaparthi cmullaparthi Merge pull request #75 from norton/dev
Add worker_process_options for start and start_link purposes
67d3bcb

@cmullaparthi cmullaparthi merged commit 67d3bcb into cmullaparthi:master Nov 19, 2012

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