Initial refactor of routers #5

Merged
merged 10 commits into from Apr 10, 2012

Projects

None yet

2 participants

Contributor

This is not code-complete

I want to open up what I've refactored for commenting/review before completing it

Contributor

I might rename this to ensure_valid_db_num since it both verifies the DB number and converts it to an int.

John Watson added some commits Apr 6, 2012
@Fluxx Fluxx and 1 other commented on an outdated diff Apr 10, 2012
nydus/db/base.py
- # If we only had one db to query, we simply return that res
- if len(results) == 1:
- return results[0]
- else:
- return results
+ if retry == self.max_connection_retries - 1:
+ raise self.MaxRetriesExceededError(e)
+
+ conn = self._connections_for(attr, retry_for=conn.num, *args, **kwargs)[0]
Fluxx
Fluxx Apr 10, 2012 Contributor

This is purely a readability thing, but I think it's more clear what's going on if you actually include the else here and make it a full conditional, so:

if not self.router.retryable:
    raise e
elif retry == self.max_connection_retries - 1:
    raise self.MaxRetriesExceededError(e)
else:
    conn = self._connections_for(attr, retry_for=conn.num, *args, **kwargs)[0]

Also, I might move all that logic into its own method called like attempt_to_retry_connection, and then the except block only needs to call that. Right now there is a lot of nesting of for loops, conditionals, try/excepts it's harder to read.

dctrwatson
dctrwatson Apr 10, 2012 Contributor

I'm hesitant to move it into its own method since the code isn't used anywhere else

Fluxx
Fluxx Apr 10, 2012 Contributor

Well, the point of the refactor was more for readability, but it's not a huge deal.

@Fluxx Fluxx and 1 other commented on an outdated diff Apr 10, 2012
nydus/db/routers/base.py
+ self.flush_down_connections()
+
+ if 'retry_for' in kwargs:
+ self.mark_connection_down(kwargs['retry_for'])
+
+ return key
+
+ def _route(self, cluster, attr, key, *args, **kwargs):
+ now = time.time()
+
+ for i in xrange(len(cluster)):
+ db_num = self._hosts_cycler.next()
+
+ marked_down_at = self._down_connections.get(db_num, 0)
+
+ if marked_down_at + self.retry_timeout <= now:
Fluxx
Fluxx Apr 10, 2012 Contributor

Are you assuming that marked_down_at will be set to 0, and that marked_down_at + self.retry_timeout <= now? If so, That works in this situation, but will break for high values of self.retry_timeout, like if someone sets it to infinity. I think it's probably more clear and better to do this:

marked_down_at = self._down_connections.get(db_num, False)

if not marked_down_at or (marked_down_at + self.retry_timeout <= now):

Also, shouldn't we be giving a host back if we're >= now?

dctrwatson
dctrwatson Apr 10, 2012 Contributor

gte now means the self.retry_timeout hasn't occurred yet
now = 1334087011.951727
retry_timeout = 30
marked_down_at = 1334087001.951727 (10 seconds ago)

marked_down_at + retry_timeout == 1334087031.951727 (20 seconds from now)

I originally had it >= now and it broke the retry tests... I had to do the above to figure out what was wrong

Fluxx
Fluxx Apr 10, 2012 Contributor

Ah, understood.

@Fluxx Fluxx commented on the diff Apr 10, 2012
nydus/db/routers/base.py
+
+class RoundRobinRouter(BaseRouter):
+ """
+ Basic retry router that performs round robin
+ """
+
+ # Raised if all hosts in the hash have been marked as down
+ class HostListExhausted(Exception):
+ pass
+
+ class InvalidDBNum(Exception):
+ pass
+
+ # If this router can be retried on if a particular db index it gave out did
+ # not work
+ retryable = True
Fluxx
Fluxx Apr 10, 2012 Contributor

Do we want this router to be retryable by default?

dctrwatson
dctrwatson Apr 10, 2012 Contributor

I think having the availability feature of nydus enabled by default is a good idea. Since using roundrobin I would want to cycle through the hosts until I get an available one.

@Fluxx Fluxx commented on the diff Apr 10, 2012
tests/nydus/db/connections/tests.py
@@ -103,14 +102,14 @@ def test_get_conn(self):
hosts={0: c, 1: c2},
)
self.assertEquals(p.get_conn(), [c, c2])
- self.assertEquals(p.get_conn('foo'), [c, c2])
Fluxx
Fluxx Apr 10, 2012 Contributor

These changed cause BaseRouter now always returns 1 host?

@Fluxx Fluxx commented on the diff Apr 10, 2012
tests/nydus/db/routers/tests.py
-class ConsistentHashingRouterTest(BaseTest):
+ def test_get_dbs_iterable(self):
Fluxx
Fluxx Apr 10, 2012 Contributor

I might test that it responds to next and/or __iter__ instead.

dctrwatson
dctrwatson Apr 10, 2012 Contributor

Why test for those methods individually over making sure it's not some subclass of Iterable?

dctrwatson
dctrwatson Apr 10, 2012 Contributor

Actually,
collections.Iterable is an ABC, so this returns true if the object has iter

I can change it to collections.Iterator if we want to make sure iter AND next exist

Fluxx
Fluxx Apr 10, 2012 Contributor

So I read up more on ABCs in Python and, while I personally disagree with it's preference of "inspection over invocation," ABC is the preferred/more Pythonic way to go so this is good.

@Fluxx Fluxx commented on the diff Apr 10, 2012
tests/nydus/db/routers/tests.py
- def setUp(self):
- self.router = ConsistentHashingRouter()
- self.hosts = dict((i, DummyConnection(i)) for i in range(5))
- self.cluster = Cluster(router=self.router, hosts=self.hosts)
+ def test_get_dbs_unabletosetuproute(self):
+ with patch.object(self.router, '_setup_router', return_value=False):
+ with self.assertRaises(BaseRouter.UnableToSetupRouter):
Fluxx
Fluxx Apr 10, 2012 Contributor

Oh cool I didn't know you could do this with assertRaises!

dctrwatson
dctrwatson Apr 10, 2012 Contributor

Ya, I think it's much prettier than the .assertRaises(exception, method, args, kwargs) of unittest

@Fluxx Fluxx commented on the diff Apr 10, 2012
tests/nydus/db/routers/tests.py
- def get_db(self, **kwargs):
- kwargs.setdefault('cluster', self.cluster)
- return self.router.get_db(func='info', **kwargs)
+ def test_offers_router_interface(self):
+ self.assertTrue(callable(self.router.get_dbs))
Fluxx
Fluxx Apr 10, 2012 Contributor

If arguments accepted are important for this interface, you could also check function arity too :)

@Fluxx Fluxx commented on an outdated diff Apr 10, 2012
tests/nydus/db/routers/tests.py
-class RoundRobinTest(BaseTest):
+
+class BaseBaseRouterTest(BaseRouterTest):
+ def test__setup_router(self):
+ self.assertTrue(self.router._setup_router(self.cluster))
+
+ def test__pre_routing(self):
Fluxx
Fluxx Apr 10, 2012 Contributor

If you can, I'd update these method names to explain what the expected behavior is. So...

def test_pre_routing_returns_passed_in_key():

Or something like that.

@Fluxx Fluxx commented on the diff Apr 10, 2012
tests/nydus/db/routers/tests.py
- def test_returns_sequence_with_one_item_when_given_key(self):
- self.assert_(len(self.get_db(key='foo')) is 1)
+ def test_mark_connection_up(self):
+ db_num = 0
+
+ self.router.mark_connection_down(db_num)
+
Fluxx
Fluxx Apr 10, 2012 Contributor

Add a self.assertIn(db_num, self.router._down_connections) in between here to show that the connection was marked as down. Otherwise, I could replace the implementation of mark_connection_down and mark_connection_up with pass and this test would pass.

Contributor
Fluxx commented Apr 10, 2012

So everything looks good to me. If you didn't have anything else, feel free to merge this pull request in and close it :)

@dctrwatson dctrwatson merged commit 825f52e into master Apr 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment