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

Consider forking twitter's hpack #4403

Closed
nmittler opened this issue Oct 28, 2015 · 18 comments
Closed

Consider forking twitter's hpack #4403

nmittler opened this issue Oct 28, 2015 · 18 comments

Comments

@nmittler
Copy link
Member

In order to be able to more easily incorporate performance improvements going forward, we should consider forking twitter's hpack implementation and just making it part of Netty.

@normanmaurer @louiscryan @Scottmitch WDYT?

@normanmaurer
Copy link
Member

@nmittler I'm not sure yet... trying to find out at the moment who is maintaining it @ twitter atm.

@nmittler
Copy link
Member Author

@normanmaurer ok, although the fact that you have to do that suggests it to be a problem :).

The project hasn't had a commit since June (and < 60 commits total) ... not a good sign :/

@buchgr
Copy link
Contributor

buchgr commented Oct 28, 2015

afaik none of the guys works at Twitter anymore, and Twitter Open Source got killed recently as well.

@louiscryan
Copy link
Contributor

Jeff is still doing reviews.

e.g. twitter/hpack#28

it may just be that they haven't done anything because no one asked them to (they have no open issues after all) so it's by no means dead. I think what would be most useful would be to list out what we think the primary benefits of having an internal impl might be vs using twitters aside from project health issues:

I don't think buffer pooling is a significant concern here as most headers are small but I could be convinced otherwise...

@louiscryan
Copy link
Contributor

@jpinner Hey Jeff, should we worry about your bus no. ?

@jpinner
Copy link

jpinner commented Oct 29, 2015

There are no maintainers of the library at Twitter anymore. I still have commit access but would need to ask my contacts at twitter to publish a new release.

In terms of bus factor, there's still @wgallagher but that's about it. The more people familiar with the code the better.

As for number and timing of commits, velocity was higher back when we were designing hpack/http2 and trailed off once the RFC was published.

@Scottmitch
Copy link
Member

My vote is to fork. Seems like the project maintenance path is in limbo and we have been kicking around the idea (#3597) of changing the APIs for a while.

@louiscryan
Copy link
Contributor

@jpinner Who are the direct users of the library these days aside from Netty and OkHttp?

@louiscryan
Copy link
Contributor

Actually it looks like okhttp forked too. Given that I'm OK with forking too

@jpinner
Copy link

jpinner commented Oct 30, 2015

@louiscryan only other thing i know of is https://github.com/twitter/netty-http2 -- i'm fine with you guys forking it btw

@Scottmitch
Copy link
Member

/cc @yschimke @madster26 ... looks like they are still contributing to twitter's netty-http2 implementation. Do you guys plan to move toward Netty's codec-http2?

@yschimke
Copy link
Contributor

I think it makes a lot of sense to fork the hpack project and make it part of the broader Netty project.

Regarding Twitter's maintenance of the netty-http2 library, the recent changes I made are a stepping stone to using netty 4.1. When we are on 4.1 then we will be able to adopt Netty's codec-http2. AFAIK no-one in Twitter plans to actively maintain the netty-http2 library considering Netty's official codec-http2.

@normanmaurer
Copy link
Member

@yschimke thanks for the info!

@nmittler
Copy link
Member Author

nmittler commented Nov 3, 2015

Ok great, thanks everyone! Sounds like we've agreed to move forward.

@buchgr would you have bandwidth and/or interest in taking a crack at this?

@buchgr
Copy link
Contributor

buchgr commented Nov 3, 2015

@nmittler yes if it's not high priority i.e. can't guarantee that I ll have it done by christmas. working on my thesis right now and trying to stay on top of classes. so can't guarantee fast delivery, only eventual delivery :).

@nmittler
Copy link
Member Author

nmittler commented Nov 4, 2015

@madongfly FYI

@nmittler
Copy link
Member Author

nmittler commented Nov 4, 2015

FYI, I've made a PR to import fork hpack under netty: #4440

nmittler pushed a commit to nmittler/netty that referenced this issue Nov 14, 2015
Motivation:

The twitter hpack project does not have the support that it used to have.  See discussion here: netty#4403.

Modifications:

Created a new module in Netty and copied the latest from twitter hpack master.

Result:

Netty no longer depends on twitter hpack.
nmittler pushed a commit that referenced this issue Nov 14, 2015
Motivation:

The twitter hpack project does not have the support that it used to have.  See discussion here: #4403.

Modifications:

Created a new module in Netty and copied the latest from twitter hpack master.

Result:

Netty no longer depends on twitter hpack.
nmittler pushed a commit that referenced this issue Nov 14, 2015
Motivation:

The twitter hpack project does not have the support that it used to have.  See discussion here: #4403.

Modifications:

Created a new module in Netty and copied the latest from twitter hpack master.

Result:

Netty no longer depends on twitter hpack.
@nmittler
Copy link
Member Author

Closing via #4440

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants