KazooClient hosts list should accept a list of multiple endpoints. #424

eribeiro · 2017-03-28T19:19:39Z

Closes #411

jeffwidman · 2017-03-31T20:32:17Z

Looks fantastic, nice job.

I'm not super excited about a list supporting both strings and tuples as formats, but OTOH while the strings are extremely common format for most libraries the tuples are very explicit, so it is handy to have both. So I support your decision to support both formats.

Could you add two more test cases where the hostname is an IP address and not a domain name?
I'd make one a IPv4 and one an IPv6 address just because people often split name/port on a : but IPv6 complicates that... and the test case protects in case anyone refactors this code down the road.

eribeiro · 2017-04-03T12:19:08Z

@jeffwidman Thanks for the comment, very insightful. Gonna add more test cases. 👍

jeffwidman · 2017-04-03T19:23:41Z

Some test failures. The python 2.6 failure is probably acceptable breakage as it doesn't support ipv6, that's up to the maintainers, but the inconsistent use of tabs/spaces should get fixed before merging.

eribeiro · 2017-04-04T18:15:19Z

@jeffwidman Thanks, gonna fix the inconsistency asap. Even on python 2.7, I've found that urllib_parse.urlsplit still have many problems parsing the IPv6 addresses. 🤔

eribeiro · 2017-04-27T01:02:46Z

@jeffwidman Hi, I tried to fix the inconsistencies between tabs/spaces. Hope everything is fine now, but let me know if something skipped, please 😃

@harlowja Does this PR makes sense? Any other file to change? Please, let me know.

Best regards!

jeffwidman · 2017-04-27T21:53:49Z

LGTM. Spot-checked a few test failures and they are python 2.6 choking on IPv6. Personally, I think this lib should solve that by dropping support for python 2, but I'm not one of the maintainers, so that's up them.

jeffwidman · 2017-05-17T00:44:45Z

@harlowja Any comment on this?

bbangert · 2017-06-01T00:01:51Z

We can't drop support for Python 2, though I would like to drop support for Python 2.6 as that should be long dead. Can this be made to work for Python 2.6?

bbangert · 2017-06-01T01:34:02Z

@jeffwidman you seem most active on github and this PR, would you be up for fixing this to work on Python 2.6? If so I can include it in the next release.

As is, I'm closing this for now, but please re-open it if changes are made that allow this to be merged.

jeffwidman · 2017-06-01T20:29:18Z

@bbangert Thanks for looking at this.

Like the other PR's that you just closed that I commented on, I do think it's a bit premature to close this one as well.

There was zero feedback on this from any of the maintainers until last night, so I don't blame @eribeiro for not spending any time on this until he knew whether it actually had a chance of getting merged.

I've literally never seen another project where a PR in process is marked as closed like this when the maintainers are interested in it, but just requesting further updates. Typically my experience is that marking something as Closed means the maintainers are rejecting it as a bad idea or such a poorly coded implementation that it's not worth working with the submitter to update it.

Personally, I'd prefer to help @eribeiro finish this by giving feedback etc rather than me finishing it as it's better in the long run to encourage someone new to open source, plus he deserves the credit as he already did the brunt of the work. So @eribeiro if you want to work on this, I'm happy to help you with reviews or answering any questions so that this is in a place that it can be merged.

jeffwidman · 2017-06-01T20:32:54Z

Also, @bbangert you requested 2.6 support above, but I see in 2faba9f you dropped official support for 2.6, so does this PR actually still need to be updated for 2.6?

bbangert · 2017-06-01T20:40:23Z

Sorry about that, due to the backlog of PR's kazoo ended up in a bad situation where there's just so many PR's I have no way to track which ones people are interested in updating, and which ones people aren't. The longer the PR list, the less anyone wanted to maintain Kazoo at all, so clearing them out so that it was clear what was actionable and what wasn't feels like the only sane UX hack around a bad usability issue. (If only Github would let me 'snooze' a PR so that it can easily be filtered out).

If you have any suggestions for alternative ways to filter out PR's that are desired but out-of-date, I'd love to use that as it wasn't my intention at all to insult/annoy anyone. This was the most immediate and obvious way to filter the list of actionable PR's that I could see, so I went that route. Sorry again about not being able to look into this earlier.

bbangert · 2017-06-01T20:41:14Z

Regarding 2.6 support, I'm fine ignoring that now that we'll be dropping Python 2.6 support. I think the PR only needs a rebase and it'll be good to go. I'll re-open it and see if GH can automatically rebase it.

bbangert

👍

jeffwidman · 2017-06-01T20:49:27Z

Sorry about that, due to the backlog of PR's kazoo ended up in a bad situation where there's just so many PR's I have no way to track which ones people are interested in updating, and which ones people aren't. The longer the PR list, the less anyone wanted to maintain Kazoo at all, so clearing them out so that it was clear what was actionable and what wasn't feels like the only sane UX hack around a bad usability issue. (If only Github would let me 'snooze' a PR so that it can easily be filtered out).

I completely understand. I help maintain a number of projects that accumulate this kind of crufty backlog, and I myself go through and close old PR's that have been sitting idle longer than 6-12 months, it's just that I try to give folks a gentle nudge first, especially if it looks like they're first-time contributors to open source.

bbangert · 2017-06-01T20:54:03Z

Oh, the only other update this needs is the commit needs to be altered to the style in the CONTRIBUTING doc. Otherwise the change won't be included in the CHANGES.md file. If the original author needs any help updating the commit and force pushing I'd be happy to help, a rebase and commit update should also remove the need for my merge commit.

bbangert

Needs to have the commit updated per the CONTRIBUTING guidelines. I can update if desired, but I'd like @eribeiro to retain credit for this, so if you're still interested @eribeiro I'd be happy to help you update the commit as needed. I've re-opened this so that we can verify the tests pass now that Python 2.6 was dropped and it looks good!

This is what I'd expect the commit to look like:

feat(core): allow multiple endpoints in KazooClient hosts arg

The hosts arg to KazooClient now supports a list of multiple endpoints.

Closes #411

bbangert · 2017-06-07T20:50:08Z

@eribeiro Did you want to update this?

jeffwidman · 2017-06-08T01:37:33Z

I haven't forgotten about this either, and if @eribeiro doesn't drop in, I am happy to update this as well.

houndci-bot · 2017-06-11T16:47:04Z

kazoo/tests/test_hosts.py

+        self.assertEquals(expected2, chroot)
+
+
+        hosts, chroot = collect_hosts(['zk01:2181', 'zk02:2181', 'zk03:2181', '/test'])


too many blank lines (2)
line too long (87 > 79 characters)

houndci-bot · 2017-06-11T16:47:04Z

kazoo/tests/test_hosts.py

+        self.assertEquals(None, chroot)
+
+
+        hosts, chroot = collect_hosts(['zk01:2181', 'zk02:2181', 'zk03:2181'])


too many blank lines (2)

houndci-bot · 2017-06-11T16:47:05Z

kazoo/tests/test_hosts.py

+    def test_hosts_list(self):
+
+        hosts, chroot = collect_hosts('zk01:2181, zk02:2181, zk03:2181')
+        expected1 = [('zk01',2181), ('zk02', 2181), ('zk03', 2181)]


missing whitespace after ','

houndci-bot · 2017-06-11T16:47:05Z

kazoo/tests/test_hosts.py

+        self.assertEquals(None, chroot)
+
+
+    def test_hosts_list(self):


too many blank lines (2)

houndci-bot · 2017-06-11T16:47:05Z

kazoo/tests/test_hosts.py

+        self.assertEquals(None, chroot)
+
+
+    def test_ipv6(self):


too many blank lines (2)

houndci-bot · 2017-06-11T16:47:05Z

kazoo/tests/test_hosts.py

+        self.assertEquals([('127.0.0.1', 2181), ('192.168.1.2', 2181), ('132.254.111.10', 2181)], hosts)
+        self.assertEquals(None, chroot)
+
+        hosts, chroot = collect_hosts(['127.0.0.1:2181', '192.168.1.2:2181', '132.254.111.10:2181'])


line too long (100 > 79 characters)

houndci-bot · 2017-06-11T16:47:05Z

kazoo/tests/test_hosts.py

+
+    def test_ipv4(self):
+        hosts, chroot = collect_hosts('127.0.0.1:2181, 192.168.1.2:2181, 132.254.111.10:2181')
+        self.assertEquals([('127.0.0.1', 2181), ('192.168.1.2', 2181), ('132.254.111.10', 2181)], hosts)


line too long (104 > 79 characters)

houndci-bot · 2017-06-11T16:47:05Z

kazoo/tests/test_hosts.py

+class HostsTestCase(TestCase):
+
+    def test_ipv4(self):
+        hosts, chroot = collect_hosts('127.0.0.1:2181, 192.168.1.2:2181, 132.254.111.10:2181')


line too long (94 > 79 characters)

houndci-bot · 2017-06-11T16:47:05Z

kazoo/tests/test_hosts.py

@@ -0,0 +1,48 @@
+import sys


'sys' imported but unused

houndci-bot · 2017-06-11T16:47:05Z

kazoo/hosts.py

-    """Collect a set of hosts and an optional chroot from a string."""
-    host_ports, chroot = hosts.partition("/")[::2]
-    chroot = "/" + chroot if chroot else None
+    """Collect a set of hosts and an optional chroot from a string or a list of strings.


line too long (84 > 79 characters)

eribeiro · 2017-06-11T16:50:34Z

@jeffwidman @bbangert Hey, thanks for taking your time to discuss and instruct me on improving this PR. It is really appreciated! 😃

Well, I have changed the commit message to the one indicated by bbangert as well as rebased the branch to the master. Please, let me know if there's any other pending work on this PR. I am glad to make any required changes.

houndci-bot · 2017-06-11T17:02:22Z

kazoo/tests/test_hosts.py

+        self.assertEquals(expected2, chroot)
+
+
+        hosts, chroot = collect_hosts(['zk01:2181',


too many blank lines (2)

houndci-bot · 2017-06-11T17:02:22Z

kazoo/hosts.py

-    host_ports, chroot = hosts.partition("/")[::2]
-    chroot = "/" + chroot if chroot else None
+    """
+       Collect a set of hosts and an optional chroot from 


trailing whitespace

The hosts arg to KazooClient now supports a list of multiple endpoints. Closes #411

bbangert · 2017-06-12T00:16:16Z

@eribeiro thanks! nice work!

eribeiro · 2017-06-13T00:32:04Z

@jeffwidman @bbangert Thanks a lot for your support and patience. It meant a lot to me. 👍

bbangert closed this Jun 1, 2017

bbangert reopened this Jun 1, 2017

bbangert approved these changes Jun 1, 2017

View reviewed changes

bbangert requested changes Jun 1, 2017

View reviewed changes

houndci-bot reviewed Jun 11, 2017

View reviewed changes

feat(core): allow multiple endpoints in KazooClient hosts arg

72a8d96

The hosts arg to KazooClient now supports a list of multiple endpoints. Closes #411

bbangert approved these changes Jun 12, 2017

View reviewed changes

bbangert merged commit 1956bab into python-zk:master Jun 12, 2017

jeffwidman mentioned this pull request Jul 25, 2017

[kafka_consumer] Switch ZK example from string to list DataDog/integrations-core#624

Merged

jeffwidman mentioned this pull request Jul 25, 2017

If kafka_consumer.py can accept multiple ZK/Kafka servers, document it DataDog/dd-agent#2943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KazooClient hosts list should accept a list of multiple endpoints. #424

KazooClient hosts list should accept a list of multiple endpoints. #424

eribeiro commented Mar 28, 2017

jeffwidman commented Mar 31, 2017

eribeiro commented Apr 3, 2017

jeffwidman commented Apr 3, 2017

eribeiro commented Apr 4, 2017

eribeiro commented Apr 27, 2017

jeffwidman commented Apr 27, 2017

jeffwidman commented May 17, 2017

bbangert commented Jun 1, 2017 •

edited

Loading

bbangert commented Jun 1, 2017

jeffwidman commented Jun 1, 2017 •

edited

Loading

jeffwidman commented Jun 1, 2017

bbangert commented Jun 1, 2017

bbangert commented Jun 1, 2017

bbangert left a comment

jeffwidman commented Jun 1, 2017 •

edited

Loading

bbangert commented Jun 1, 2017

bbangert left a comment •

edited

Loading

bbangert commented Jun 7, 2017

jeffwidman commented Jun 8, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

eribeiro commented Jun 11, 2017

houndci-bot Jun 11, 2017

houndci-bot Jun 11, 2017

bbangert commented Jun 12, 2017

eribeiro commented Jun 13, 2017

		self.assertEquals(expected2, chroot)


		hosts, chroot = collect_hosts(['zk01:2181', 'zk02:2181', 'zk03:2181', '/test'])

		self.assertEquals(None, chroot)


		hosts, chroot = collect_hosts(['zk01:2181', 'zk02:2181', 'zk03:2181'])

KazooClient hosts list should accept a list of multiple endpoints. #424

KazooClient hosts list should accept a list of multiple endpoints. #424

Conversation

eribeiro commented Mar 28, 2017

jeffwidman commented Mar 31, 2017

eribeiro commented Apr 3, 2017

jeffwidman commented Apr 3, 2017

eribeiro commented Apr 4, 2017

eribeiro commented Apr 27, 2017

jeffwidman commented Apr 27, 2017

jeffwidman commented May 17, 2017

bbangert commented Jun 1, 2017 • edited Loading

bbangert commented Jun 1, 2017

jeffwidman commented Jun 1, 2017 • edited Loading

jeffwidman commented Jun 1, 2017

bbangert commented Jun 1, 2017

bbangert commented Jun 1, 2017

bbangert left a comment

Choose a reason for hiding this comment

jeffwidman commented Jun 1, 2017 • edited Loading

bbangert commented Jun 1, 2017

bbangert left a comment • edited Loading

Choose a reason for hiding this comment

bbangert commented Jun 7, 2017

jeffwidman commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eribeiro commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbangert commented Jun 12, 2017

eribeiro commented Jun 13, 2017

bbangert commented Jun 1, 2017 •

edited

Loading

jeffwidman commented Jun 1, 2017 •

edited

Loading

jeffwidman commented Jun 1, 2017 •

edited

Loading

bbangert left a comment •

edited

Loading