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

Python 3 Compatibility (Part 2) #111

Closed
wants to merge 10 commits into from
13 changes: 13 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,25 @@ doc:
@echo Generated documentation: "file://"$$(readlink -f doc/build/html/index.html)
@echo

doc3:
python3 setup.py build_sphinx
@echo
@echo Generated documentation: "file://"$$(readlink -f doc/build/html/index.html)
@echo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part is not really needed. a separate tox environment for building the docs is perhaps a better solution.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what tox is. Do whatever you think is reasonable for your development needs.

test:
-find coverage/ -mindepth 1 -delete
python $$(which nosetests) $${TESTS}

test3:
-find coverage/ -mindepth 1 -delete
python3 $$(which nosetests3) $${TESTS}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this target should go. a separate tox environment is the preferred way to test against multiple python versions.

these makefile targets only exist because it makes development from within a virtualenv easier (no need to run tox which is slower). and within a virtualenv, python points to the correct one: there's no python vs python3.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I have no knowledge of tox. I did, however, use the test3 target extensively to progressively test my changes in Python 3.

clean:
find . -name '*.py[co]' -delete

dist: test
python setup.py sdist

dist3: test3
python3 setup.py sdist
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this target is not needed at all.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I've been using python3 in my invocations for building the package using stdeb, and blindly copied it in for creating the source distribution.

6 changes: 0 additions & 6 deletions TODO.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,3 @@ future, depending on time, demand, and technical possibilities.
* Port HappyBase over to the (still experimental) HBase Thrift2 API when it
becomes mainstream, and expose more of the underlying features nicely in the
HappyBase API.

* Python 3 support. This would be trivial for HappyBase, now that the
underlying Thrift library is Python 3 compatible. `Track`_ this
issue online.

.. _Track: https://github.com/wbolster/happybase/issues/40
11 changes: 7 additions & 4 deletions doc/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
# All configuration values have a default; values that are commented out
# serve to show the default.

import sys, os
import imp
from os.path import dirname, join

# If extensions (or modules to document with autodoc) are in another directory,
# add these directories to sys.path here. If the directory is relative to the
Expand Down Expand Up @@ -48,11 +49,13 @@
# built documents.
#
# The short X.Y version.
execfile(os.path.join(os.path.dirname(__file__), '../happybase/_version.py'))
version = __version__

version_path = join(dirname(__file__), '../happybase/_version.py')
with open(version_path) as fp:
version = imp.load_source("_version", version_path, fp).__version__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imp.load_source() is not public api in python 3, and the imp module is deprecated and pending removal...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, the setup.py has this:

__version__ = None
exec(open('happybase/_version.py', 'r').read())

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exec() seems like overkill. If I had my way, I would likely put the version (and any other metadata about the module) into a separate file (json, ini, etc.), and read the data from there.


# The full version, including alpha/beta/rc tags.
release = __version__
release = version

# The language for content autogenerated by Sphinx. Refer to documentation
# for a list of supported languages.
Expand Down
18 changes: 1 addition & 17 deletions happybase/Hbase.thrift
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ struct ColumnDescriptor {
6:i32 bloomFilterVectorSize = 0,
7:i32 bloomFilterNbHashes = 0,
8:bool blockCacheEnabled = 0,
9:i32 timeToLive = -1
9:i32 timeToLive = 0x7fffffff
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

am i right in assuming that this is a unmodified upstream version of the thrift file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct.

}

/**
Expand Down Expand Up @@ -906,22 +906,6 @@ service Hbase {
1:ScannerID id
) throws (1:IOError io, 2:IllegalArgument ia)

/**
* Get the row just before the specified one.
*
* @return value for specified row/column
*/
list<TCell> getRowOrBefore(
/** name of table */
1:Text tableName,

/** row key */
2:Text row,

/** column name */
3:Text family
) throws (1:IOError io)

/**
* Get the regininfo for the specified row. It scans
* the metatable to find region's start and end keys.
Expand Down
4 changes: 2 additions & 2 deletions happybase/batch.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def _reset_mutations(self):

def send(self):
"""Send the batch to the server."""
bms = [BatchMutation(row, m) for row, m in self._mutations.iteritems()]
bms = [BatchMutation(row, m) for row, m in self._mutations.items()]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes copies into a new list which iterated over and then thrown away in python 2 :(

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I thought about creating a separate utility function just for iterating over items:

if sys.version_info >= (3,):
    def iteritems(d):
        return d.items()
else:
    def iteritems(d):
        return d.iteritems()

PEP 469 recommends this approach, though also mentions that six and future.utils provide the same functionality.

if not bms:
return

Expand Down Expand Up @@ -80,7 +80,7 @@ def put(self, row, data, wal=None):
column=column,
value=value,
writeToWAL=wal)
for column, value in data.iteritems())
for column, value in data.items())

self._mutation_count += len(data)
if self._batch_size and self._mutation_count >= self._batch_size:
Expand Down
41 changes: 20 additions & 21 deletions happybase/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
DEFAULT_COMPAT = '0.96'
DEFAULT_PROTOCOL = 'binary'


class Connection(object):
"""Connection to an HBase Thrift server.

Expand Down Expand Up @@ -97,26 +96,26 @@ class Connection(object):
:param int port: The port to connect to
:param int timeout: The socket timeout in milliseconds (optional)
:param bool autoconnect: Whether the connection should be opened directly
:param str table_prefix: Prefix used to construct table names (optional)
:param str table_prefix_separator: Separator used for `table_prefix`
:param bytes table_prefix: Prefix used to construct table names (optional)
:param bytes table_prefix_separator: Separator used for `table_prefix`
:param str compat: Compatibility mode (optional)
:param str transport: Thrift transport mode (optional)
"""
def __init__(self, host=DEFAULT_HOST, port=DEFAULT_PORT, timeout=None,
autoconnect=True, table_prefix=None,
table_prefix_separator='_', compat=DEFAULT_COMPAT,
table_prefix_separator=b'_', compat=DEFAULT_COMPAT,
transport=DEFAULT_TRANSPORT, protocol=DEFAULT_PROTOCOL):

if transport not in THRIFT_TRANSPORTS:
raise ValueError("'transport' must be one of %s"
% ", ".join(THRIFT_TRANSPORTS.keys()))

if table_prefix is not None \
and not isinstance(table_prefix, basestring):
raise TypeError("'table_prefix' must be a string")
and not isinstance(table_prefix, bytes):
raise TypeError("'table_prefix' must be a byte string")

if not isinstance(table_prefix_separator, basestring):
raise TypeError("'table_prefix_separator' must be a string")
if not isinstance(table_prefix_separator, bytes):
raise TypeError("'table_prefix_separator' must be a byte string")

if compat not in COMPAT_MODES:
raise ValueError("'compat' must be one of %s"
Expand Down Expand Up @@ -213,7 +212,7 @@ def table(self, name, use_prefix=True):
argument to the :py:class:`Connection` constructor for more
information.

:param str name: the name of the table
:param bytes name: the name of the table
:param bool use_prefix: whether to use the table prefix (if any)
:return: Table instance
:rtype: :py:class:`Table`
Expand All @@ -233,13 +232,13 @@ def tables(self):
tables that have the specified prefix will be listed.

:return: The table names
:rtype: List of strings
:rtype: List of byte strings
"""
names = self.client.getTableNames()

# Filter using prefix, and strip prefix from names
if self.table_prefix is not None:
prefix = self._table_name('')
prefix = self._table_name(b'')
offset = len(prefix)
names = [n[offset:] for n in names if n.startswith(prefix)]

Expand All @@ -248,7 +247,7 @@ def tables(self):
def create_table(self, name, families):
"""Create a table.

:param str name: The table name
:param bytes name: The table name
:param dict families: The name and options for each column family

The `families` argument is a dictionary mapping column family
Expand Down Expand Up @@ -288,16 +287,16 @@ def create_table(self, name, families):
% name)

column_descriptors = []
for cf_name, options in families.iteritems():
for cf_name, options in families.items():
if options is None:
options = dict()

kwargs = dict()
for option_name, value in options.iteritems():
for option_name, value in options.items():
kwargs[pep8_to_camel_case(option_name)] = value

if not cf_name.endswith(':'):
cf_name += ':'
if not cf_name.endswith(b':'):
cf_name += b':'
kwargs['name'] = cf_name

column_descriptors.append(ColumnDescriptor(**kwargs))
Expand All @@ -314,7 +313,7 @@ def delete_table(self, name, disable=False):
deleted. If the `disable` argument is `True`, this method first
disables the table if it wasn't already and then deletes it.

:param str name: The table name
:param bytes name: The table name
:param bool disable: Whether to first disable the table if needed
"""
if disable and self.is_table_enabled(name):
Expand All @@ -326,23 +325,23 @@ def delete_table(self, name, disable=False):
def enable_table(self, name):
"""Enable the specified table.

:param str name: The table name
:param bytes name: The table name
"""
name = self._table_name(name)
self.client.enableTable(name)

def disable_table(self, name):
"""Disable the specified table.

:param str name: The table name
:param bytes name: The table name
"""
name = self._table_name(name)
self.client.disableTable(name)

def is_table_enabled(self, name):
"""Return whether the specified table is enabled.

:param str name: The table name
:param bytes name: The table name

:return: whether the table is enabled
:rtype: bool
Expand All @@ -353,7 +352,7 @@ def is_table_enabled(self, name):
def compact_table(self, name, major=False):
"""Compact the specified table.

:param str name: The table name
:param bytes name: The table name
:param bool major: Whether to perform a major compaction.
"""
name = self._table_name(name)
Expand Down
Loading