-
Notifications
You must be signed in to change notification settings - Fork 180
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
HBASE-26354 [hbase-connectors] Added python client for HBase thrift service #86
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🎊 +1 overall
This message was automatically generated. |
Have fixed all the whitespace problem. Could someone have a look of this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a reference in the top level README for the project. Also needs to be hooked into the main build process or we need to make we a) include instructions for building and b) update the project release scripts to follow those instructions when a release manager goes to make RCs of the hbase-connectors repo.
thrift-python/LICENSE
Outdated
|
||
Apache License | ||
Version 2.0, January 2004 | ||
http://www.apache.org/licenses/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid having another copy of the LICENSE here? the top level one is meant to cover the source in the repo. if we need a copy for e.g. inclusion in some artifact can we pull that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that in the spark or kafka submodule, there is no redundant LICENSE file. Has removed.
@@ -0,0 +1,169 @@ | |||
# Project description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual files need to have a header that points out the licensing. see here for more info:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved.
thrift-python/setup.py
Outdated
long_description = f.read() | ||
|
||
setup( | ||
name='thbase', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like it if the name was less opaque. maybe hbase-thrift2
or hbase-client-thrift2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, have changed to hbase-client-thrift2.
thrift-python/setup.py
Outdated
|
||
setup( | ||
name='thbase', | ||
version='1.3.6', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far we've gone with unified releases from hbase-connectors. That is, the version here should be the same as the hbase-connectors version it is released in. ATM the in development version is 1.0.1. With this library adding we should probably bump that to 1.1 or 2.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what should be the version number. Just used 2.0 in the latest commit.
thrift-python/setup.py
Outdated
setup( | ||
name='thbase', | ||
version='1.3.6', | ||
description='HBase thrift2 client python API. Compatible with python2.7 and python3', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the module description we should use the proper trademark "Apache HBase thrift2 client ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to a more official description in the latest commit.
classifiers=[ | ||
"License :: OSI Approved :: Apache Software License", | ||
"Natural Language :: English", | ||
"Programming Language :: Python :: 2.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how long are we looking to maintain python 2.7 support? is it still commonly needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still 2.7 users. But the 2.7 users I know, are planning to upgrade to 3.x. I think we could abandon 2.7 soon.
thrift-python/setup.py
Outdated
author='', | ||
author_email='', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should be set to something like "Apache HBase Community" and "[email protected]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the latest commit.
Have added the release instructions in the README file. The release needs a PMC to register a pypi project with the same name first. |
🎊 +1 overall
This message was automatically generated. |
|
||
class ClientBase(object): | ||
""" | ||
Abstract class for both thrift1 and thrift2 client. Implemented with Observer design pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thrift2 only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Thrift2 only. As Thrift1 will be deprecated, only build up for thrift2. But it is ok to develop another thrift1 client if needed.
Any other comments on this? |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
https://issues.apache.org/jira/browse/HBASE-26354