facebookarchive / swift

An annotation-based Java library for creating Thrift serializable types and services.
Apache License 2.0
900 stars 297 forks source link

Fix thread pool setup #9

Closed dain closed 12 years ago

dain commented 12 years ago

Add a real worker thread pool.

Upgrade to Airlift 0.66 Remove old dependency on nifty-core-asf

ghost commented 12 years ago

Hey Dain,

Will one of you be able to write up a doc on how to build a thrift service with nifty + swift? And/or transition an existing one (ie, PumaReadService). Or a quick session if you prefer (but docs are better :) )

We're on java7 now in puma, and I want to move our shard and gautam's metastore (lower priority) so that everything uses it.

If I haven't expressed it--I'm so grateful to have you two (and Jax) building solid components to solve pain points for which I didn't have the time or capability to do over the last two years. Thanks.

thx, -sr

sam rash sr@fb.com

On 7/27/12 12:37 PM, "Dain Sundstrom" <reply+i-5883944-7c24b35090aac97abbb63ef15d5ea53d89e1044e-312230@reply.gith ub.com> wrote:

Add a real worker thread pool.

Upgrade to Airlift 0.66 Remove old dependency on nifty-core-asf

You can merge this Pull Request by running:

git pull https://github.com/dain/swift master

Or you can view, comment on it, or merge it online at:

https://github.com/facebook/swift/pull/9

-- Commit Summary --

  • Upgrade to Airlift 0.66
  • Remove old nifty-core-asf dependency
  • Fix thread pool setup

-- File Changes --

M pom.xml (12) M swift-service/pom.xml (2) M swift-service/src/main/java/com/facebook/swift/service/ThriftServer.java (41) M swift-service/src/main/java/com/facebook/swift/service/ThriftServerConfig. java (14)

-- Patch Links --

https://github.com/facebook/swift/pull/9.patch https://github.com/facebook/swift/pull/9.diff


Reply to this email directly or view it on GitHub: https://github.com/facebook/swift/pull/9

dain commented 12 years ago

Docs are already in the project:

https://github.com/facebook/swift/tree/master/swift-service

On Sat, Jul 28, 2012 at 11:39 PM, Sam Rash reply@reply.github.com wrote:

Hey Dain,

Will one of you be able to write up a doc on how to build a thrift service with nifty + swift? And/or transition an existing one (ie, PumaReadService). Or a quick session if you prefer (but docs are better :) )

We're on java7 now in puma, and I want to move our shard and gautam's metastore (lower priority) so that everything uses it.

If I haven't expressed it--I'm so grateful to have you two (and Jax) building solid components to solve pain points for which I didn't have the time or capability to do over the last two years. Thanks.

thx, -sr

sam rash sr@fb.com

On 7/27/12 12:37 PM, "Dain Sundstrom" <reply+i-5883944-7c24b35090aac97abbb63ef15d5ea53d89e1044e-312230@reply.gith ub.com> wrote:

Add a real worker thread pool.

Upgrade to Airlift 0.66 Remove old dependency on nifty-core-asf

You can merge this Pull Request by running:

git pull https://github.com/dain/swift master

Or you can view, comment on it, or merge it online at:

https://github.com/facebook/swift/pull/9

-- Commit Summary --

  • Upgrade to Airlift 0.66
  • Remove old nifty-core-asf dependency
  • Fix thread pool setup

-- File Changes --

M pom.xml (12) M swift-service/pom.xml (2) M swift-service/src/main/java/com/facebook/swift/service/ThriftServer.java (41) M swift-service/src/main/java/com/facebook/swift/service/ThriftServerConfig. java (14)

-- Patch Links --

https://github.com/facebook/swift/pull/9.patch https://github.com/facebook/swift/pull/9.diff


Reply to this email directly or view it on GitHub: https://github.com/facebook/swift/pull/9


Reply to this email directly or view it on GitHub: https://github.com/facebook/swift/pull/9#issuecomment-7350283

andrewcox commented 12 years ago

airlift stuff lgtm

remove nifty-core-asf dependency thought this was in a pull that already got put in, but if not go for it

thread pool setup, these changes do look good (I had made similar changes myself before I realized you had them) but why aren't we using NiftyBootstrap here? looks like there is a lot of duplicated code between this and that?

andrewcox commented 12 years ago

Also, if we don't want to just use niftybootstrap, I've got a change in my working dir from there that we really need with this: closing the channel group. Otherwise you can get into situations where the io thread pool executor times out awaiting shutdown, and the java process the tests were running in sits around like a zombie.

dain commented 12 years ago

The NiftyBootstrap doesn't fit in well with the way the rest of the code uses guice (specifically configuration).

Can you post a link to the change that fixes the zombie threads?

martint commented 12 years ago

looks good now