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

New branch for Tornado web framework support? #17

Open
tristeng opened this issue Jul 17, 2014 · 5 comments
Open

New branch for Tornado web framework support? #17

tristeng opened this issue Jul 17, 2014 · 5 comments

Comments

@tristeng
Copy link
Contributor

I've created a branch off of a fork of Pyaccumulo that I've dedicated to adding support for the Tornado web framework: https://github.com/tristeng/pyaccumulo/tree/tornado

This involved generating a different flavour of Python thrift files (makes use of Tornado's async features). As I've made updates to the Tornado related code, I've also added support to many more of the Thrift defined functions in the original Pyaccumulo code (managing users, adding table iterators, constraints, etc.).

I would have merged to master and made a pull request, but these new changes add a dependency to the Tornado library. Also, the Thrift library supports an older version of Tornado and I should really attempt to update that first before merging to master.

Would it make sense to pull my tornado branch to a tornado branch here?

@johnrfrank
Copy link
Contributor

Looks cool!

Could you say more about the bigger picture? What are you doing with tornado that motivated these improvements? Can they be absorbed in pieces, i.e. multiple separate pull requests? Or is it more monolithic?

@jatrost
Copy link
Collaborator

jatrost commented Jul 18, 2014

i think a tornado branch in the main repo may make sense as long as the changes are too drastic. if they are, then a new repo may make more sense.

@tristeng
Copy link
Contributor Author

@johnrfrank: we use Tornado as the front-end for one of our products at work and needed the ability to communicate with our Accumulo instance using Tornado's async functionality. The original Pyaccumulo library works, but any long running calls (i.e. scans) will block Tornado's IO loop. Fortunately, Thrift has a Python generator for Tornado so I started porting the original Pyaccumulo code into a Tornado-friendly library, and as we started integrating tighter with Accumulo, I started adding functions from the Thrift interface that Pyaccumulo doesn't support. I made sure to add any new functions to the original Pyaccumulo library as well. I think there are two pieces that could be absorbed; the first would be the updates to the original Pyaccumulo library (a bunch of new functions), and the second would be the tornado functionality (this would be monolithic, but it sits in its own module so shouldn't affect the original code). Maybe it would make sense to cherrypick the changes to the original library and submit those first.

@jt6211: The changes to the original Pyaccumulo are pretty straight forward, and the Tornado updates sit in their own module (pyaccumulo.tornado); they re-use some of the original wrapper classes

There are a few concerns for the Tornado side of things:

  1. The Thrift interface is based on a legacy version of Tornado and is in dire need of an update; in order to get it working, a minor (but critical patch) is required on the Thrift side. The Thrift interface was written for Tornado 2.x and 4.x was just released. I'm looking into this.
  2. Once 1) is complete, the Tornado Pyaccumulo would also need some refactoring to bring it up to speed.
  3. Finally, adding the Tornado code pulls in a dependency on Tornado. Perhaps the setup.py could be updated to allow the end-user to choose the version to install

@johnrfrank
Copy link
Contributor

+1 for separating merge requests

+1 for getting this all into master (instead of a long-lived separate branch)

@jatrost
Copy link
Collaborator

jatrost commented Jul 18, 2014

Yeah if this can live in a separate module this seems like a great addition. Thanks @tristeng

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

3 participants