Skip to content

READY: Add Timeseries support#416

Merged
borshop merged 31 commits into
masterfrom
features/lrb/rts-367
Dec 14, 2015
Merged

READY: Add Timeseries support#416
borshop merged 31 commits into
masterfrom
features/lrb/rts-367

Conversation

@lukebakken
Copy link
Copy Markdown
Contributor

  • RTS-367
  • RTS-430
  • RTS-426
  • RTS-462
  • RTS-474
  • RTS-454
  • RTS-485
  • RTS-513
  • RTS-487

Also refactoring unit tests so that individual test suites can be run via the --test-suite argument:

python2 setup.py test --test-suite=riak.tests.test_timeseries

This involved removing the hackish Pbc and Http unit test classes that would switch protocols out underneath the test suite as well as making it impossible to run an individual suite. Instead, use the RIAK_TEST_PROTOCOL environment variable to choose a protocol to test.

Include a fix so that the timeout transport option is actually passed to the HTTP connection classes when instantiated.

Comment thread commands.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why create a new class just for timeseries? Seems like you could create other classes for, say, datatypes to be consistent. Is there some special behavior here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I figured since TS isn't a real thing yet it would be nice to keep it separate and skippable.

Comment thread riak/transports/pbc/codec.py Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

leave in logging? hmm

@lukebakken lukebakken changed the title WIP: Add Timeseries support READY: Add Timeseries support Oct 27, 2015
Comment thread README.rst
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice, but don't we already have a THANKS file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah! I'll move those here ... that's what @mjbrender has had us do with the other clients. I know one unit test reads the THANKS file so I'll address that.

@hazen
Copy link
Copy Markdown

hazen commented Oct 28, 2015

@lukebakken Since you were massively reworking the test infrastructure in this PR I took the liberty of getting rid of all of the SKIP variables to make things clearer and more consistent.

Also, before it will pass buildbot's keen eye, you need to do a make lint in the buildbot directory which runs pep8 and pyflakes syntax and style checkers.

I was doing a regression test against a non-TS server to see how things worked and all is well except that HTTP security tests seem to be broken. I'll look at that later. I think I broke one security test, too, in cleaning things up. I'll return to that as well.

It looks like Time Series kills the 2i tests. I know there is some code overlap, so perhaps that is to be expected.

Also the actual Time Series tests seem to fail against the latest build of e2e/ts.

@lukebakken
Copy link
Copy Markdown
Contributor Author

Output from make lint ... seems to indicate success:

lbakken@brahms ~/Projects/basho/riak-python-client (features/lrb/rts-367=)
$ make -C buildbot lint
make: Entering directory '/home/lbakken/Projects/basho/riak-python-client/buildbot'
Collecting pep8
  Downloading pep8-1.6.2-py2.py3-none-any.whl (40kB)
    100% |████████████████████████████████| 40kB 1.5MB/s
Collecting flake8
  Downloading flake8-2.5.0-py2.py3-none-any.whl
Collecting pyflakes<1.1,>=0.8.1 (from flake8)
  Downloading pyflakes-1.0.0-py2.py3-none-any.whl (152kB)
    100% |████████████████████████████████| 155kB 3.1MB/s
Collecting mccabe<0.4,>=0.2.1 (from flake8)
  Downloading mccabe-0.3.1-py2.py3-none-any.whl
Installing collected packages: pep8, pyflakes, mccabe, flake8
Successfully installed flake8-2.5.0 mccabe-0.3.1 pep8-1.6.2 pyflakes-1.0.0
/home/lbakken/Projects/basho/riak-python-client/buildbot/../riak/tests/resources/client.crt: OK
/home/lbakken/Projects/basho/riak-python-client/buildbot/../riak/tests/resources/server.crt: OK
make: Leaving directory '/home/lbakken/Projects/basho/riak-python-client/buildbot'

@hazen
Copy link
Copy Markdown

hazen commented Oct 28, 2015

@lukebakken That's because it passes now. 😀 I cleaned up all of the warnings.

@lukebakken
Copy link
Copy Markdown
Contributor Author

@javajolt this is the first time I've run it ... did you have to make changes to get it to check out OK?

@lukebakken
Copy link
Copy Markdown
Contributor Author

Aha I'm assuming some of the whitespace / formatting changes in your commit.

Brett Hazen and others added 6 commits December 10, 2015 15:28
due to Python 3's in limitation on binary encoded strings. Also tweak
a few version compatibility features and clean up PEP8/pyflakes
warnings.
- Retire Python 2.6
- Swap 2.7.8 for 2.7.9 (PyOpenSSL version)
- Add in Python 3.5.1
- Retire Python 2.6
- Swap 2.7.8 for 2.7.9 (PyOpenSSL version)
- Add in Python 3.5.1
Comment thread setup.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't understand this requirement. If you have riak_pb in your PYTHONPATH, why do you have to have protobuf version 2.6.1? I assume if you had, say 2.5.0, surely that would work, too. Normally installing riak_pb from PyPI will get the required protobuf libraries. Also this will break with Python 3 which actually requires python3_protobuf instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found that it won't install protobuf for you if you're using PYTHONPATH to point to riak_pb. I haven't yet pushed a change that looks for riak_pb/python3/lib as well.

Without this code, you can't use PYTHONPATH to point to riak_pb. Of course, once #418 is merged none of this is necessary.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As you say, this will soon go away. I guess I'd prefer just to manually manage your own dependencies for a manual install. Seems like a temporary requirement. Or we can rip this out as part of #418.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmmm... Upon reflection, I think we could just add a protobuf dependency. Of course you'll need to do that in #418 anyway.

@hazen
Copy link
Copy Markdown

hazen commented Dec 14, 2015

👍 57c47c0

borshop added a commit that referenced this pull request Dec 14, 2015
READY: Add Timeseries support

Reviewed-by: javajolt
@hazen
Copy link
Copy Markdown

hazen commented Dec 14, 2015

@borshop merge

@borshop borshop merged commit 57c47c0 into master Dec 14, 2015
@lukebakken lukebakken deleted the features/lrb/rts-367 branch December 14, 2015 16:13
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

Successfully merging this pull request may close these issues.

3 participants