support range format in worker #325

yuokada · 2017-03-04T10:38:28Z

resolve #247

Example.

worker[01-02].example.com => [worker01.example.com, worker02.example.com]
unittest
update document
git rebase

This change is

kokosing · 2017-03-04T16:41:33Z

Please update the documentation. 04.03.2017 11:38 "Yukihiro Okada" <[email protected]> napisał(a):

resolve #247 <#247> ------------------------------ Example. - worker[01-02].example.com => [worker01.example.com, worker02.example.com] ------------------------------ You can view, comment on, or merge this pull request online at: #325 Commit Summary - Add expand_host func File Changes - *M* prestoadmin/standalone/config.py <https://github.com/prestodb/presto-admin/pull/325/files#diff-0> (21) Patch Links: - https://github.com/prestodb/presto-admin/pull/325.patch - https://github.com/prestodb/presto-admin/pull/325.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#325>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHN_-6VZcU7mppTtRS6YJ95k5xKzOyIAks5riT8lgaJpZM4MTDIv> .

kokosing · 2017-03-04T16:42:44Z

Please also add unit tests. 04.03.2017 17:41 "Grzegorz Kokosiński" <[email protected]> napisał(a):

…

Please update the documentation. 04.03.2017 11:38 "Yukihiro Okada" ***@***.***> napisał(a): > resolve #247 <#247> > ------------------------------ > > Example. > > - worker[01-02].example.com => [worker01.example.com, > worker02.example.com] > > ------------------------------ > You can view, comment on, or merge this pull request online at: > > #325 > Commit Summary > > - Add expand_host func > > File Changes > > - *M* prestoadmin/standalone/config.py > <https://github.com/prestodb/presto-admin/pull/325/files#diff-0> (21) > > Patch Links: > > - https://github.com/prestodb/presto-admin/pull/325.patch > - https://github.com/prestodb/presto-admin/pull/325.diff > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#325>, or mute the thread > <https://github.com/notifications/unsubscribe-auth/AHN_-6VZcU7mppTtRS6YJ95k5xKzOyIAks5riT8lgaJpZM4MTDIv> > . >

yuokada · 2017-03-05T05:55:33Z

OK, I make a task-list.

kokosing · 2017-03-06T09:08:05Z

prestoadmin/standalone/config.py

@@ -140,6 +143,24 @@ def validate_workers(workers):
    return workers


+def expand_host(host):


make it private by prefixing method with _

renamed to _expand_host

kokosing · 2017-03-06T09:09:09Z

prestoadmin/standalone/config.py

@@ -116,6 +118,7 @@ def validate(conf):
    except KeyError:
        pass
    else:
+        workers = [h for host in workers for h in expand_host(host)]


what if I don't want name expansion like that?
For example what if I have host names which are exactly like cdh5-1 and cdh5-2?

the function expand host when enclose with bracket.

"cdh5-[1-2].example.com" => expand "cdh5-1.example.com" => not expand

ok. That sounds good as long [ and ] are forbidden to be used in host names.

See https://en.wikipedia.org/wiki/Hostname.

The Internet standards (Requests for Comments) for protocols mandate that component hostname labels may contain only the ASCII letters 'a' through 'z' (in a case-insensitive manner), the digits '0' through '9', and the hyphen ('-'). The original specification of hostnames in RFC 952, mandated that labels could not start with a digit or with a hyphen, and must not end with a hyphen. However, a subsequent specification (RFC 1123) permitted hostname labels to start with digits. No other symbols, punctuation characters, or white space are permitted.

so we are good here.

kokosing · 2017-03-06T09:14:07Z

prestoadmin/standalone/config.py

@@ -140,6 +143,24 @@ def validate_workers(workers):
    return workers


+def expand_host(host):
+    m = re.match("(.*)\[(\d{1,})-(\d{1,})\](.*)", host)
+    if m is not None and len(m.groups()) == 4:


I think it would be nice to model it in more object-oriented manner. So it it would be possible to inject different expansions strategies.

kokosing · 2017-03-07T13:54:39Z

@yuokada Please notice that with every commit you start a travis build. presto-admin travis build requires 7 executors for about 30 minutes. These executors are shared between all the prestodb projects

yuokada · 2017-03-07T14:08:56Z

@zpao Oh, sorry. I cancel some tests.

kokosing · 2017-03-07T14:09:26Z

@yuokada thanks

kokosing

Please squash the commits and add the docs.

Good job so far!

kokosing · 2017-03-08T07:51:30Z

prestoadmin/standalone/config.py

@@ -116,6 +118,7 @@ def validate(conf):
    except KeyError:
        pass
    else:
+        workers = [h for host in workers for h in expand_host(host)]


ok. That sounds good as long [ and ] are forbidden to be used in host names.

See https://en.wikipedia.org/wiki/Hostname.

The Internet standards (Requests for Comments) for protocols mandate that component hostname labels may contain only the ASCII letters 'a' through 'z' (in a case-insensitive manner), the digits '0' through '9', and the hyphen ('-'). The original specification of hostnames in RFC 952, mandated that labels could not start with a digit or with a hyphen, and must not end with a hyphen. However, a subsequent specification (RFC 1123) permitted hostname labels to start with digits. No other symbols, punctuation characters, or white space are permitted.

so we are good here.

kokosing · 2017-03-08T07:56:07Z

tests/unit/test_expand.py

+        self.assertEqual(expected, _expand_host(input_host))
+
+
+if __name__ == "__main__":


please remove this if

kokosing · 2017-03-08T08:00:07Z

tests/unit/test_expand.py

+
+"""
+Tests the expand host func
+"""


please remove this doc

kokosing · 2017-03-08T08:04:15Z

tests/unit/test_expand.py

+        input_host = "worker0[1-2].example.com"
+        expected = ["worker01.example.com", "worker02.example.com"]
+        self.assertEqual(expected, _expand_host(input_host))
+        # self.assertEqualIgnoringOrder(expected, cmd_output)


please remove this line

missing docs

yuokada · 2017-03-08T15:50:28Z

I write a documents. Could you review it?

yuokada · 2017-03-08T15:58:22Z

The document is like this.

kokosing

minor comments, looks good, once you address them I will merge your code

@cawallin @rschlussel you may may also want to take a look

kokosing · 2017-03-09T06:58:51Z

prestoadmin/standalone/config.py

@@ -140,6 +143,24 @@ def validate_workers(workers):
    return workers


+def _expand_host(host):
+    m = re.match("(.*)\[(\d{1,})-(\d{1,})\](.*)", host)


please do not use short names like m

please replace to m to soemthing like match and num_fnt to number_format (if understand it correctly)

kokosing · 2017-03-09T07:01:45Z

prestoadmin/standalone/config.py

+            host_list = [num_fmt.format(i) for i in range(int(start), int(end) + 1)]
+            return ["{prefix}{num}{suffix}".format(
+                prefix=prefix, num=i, suffix=suffix)
+                    for i in host_list]


I would format this code as:

return ["{prefix}{num}{suffix}".format(prefix=prefix, num=i, suffix=suffix) for i in host_list]

I feel that one line is too long.

Yeah... but in that case I would prefer to either extract method or variable, splitting line does not help IMO

kokosing · 2017-03-09T07:01:56Z

prestoadmin/standalone/config.py

+        else:
+            return ["{prefix}{num}{suffix}".format(
+                prefix=prefix, num=i, suffix=suffix)
+                    for i in range(int(start), int(end) + 1)]


the same with formatting

prestoadmin/standalone/config.py:155:121: E501 line too long (129 > 120 characters)

pep8 error occurred.
https://travis-ci.org/prestodb/presto-admin/jobs/209255743

I format this code.

return ["{prefix}{num}{suffix}".format(prefix=prefix, num=i, suffix=suffix) for i in range(int(start), int(end) + 1)]

Please extract a method then. When you take a look now (when you removed line splits) it is clear that code (the one with "...".format(...)is repeated twice)

rschlussel-zz

Thanks for the PR! A few minor things.

rschlussel-zz · 2017-03-09T15:55:13Z

prestoadmin/standalone/config.py

@@ -140,6 +143,24 @@ def validate_workers(workers):
    return workers


+def _expand_host(host):


You should raise an error if the bottom of the range is greater than the top (e.g. worker[5-2]. And also add a unit test for this case.

Hi, I add a unittest: test_except_expand_host.

rschlussel-zz · 2017-03-09T15:58:20Z

docs/installation/presto-admin-configuration.rst

@@ -93,6 +93,18 @@ username and port, a sample ``config.json`` would be:
 "workers": ["slave1","slave2","slave3","slave4","slave5"]
 }

+.. NOTE:: the ``workers`` support range format.


Instead of adding a note, make this its own paragraph with a bit more description.

e.g.

You can specify a range of workers by including the number range in brackets in the worker name. For example:

(and then keep your example)

rschlussel-zz · 2017-03-09T15:58:29Z

docs/installation/presto-admin-configuration.rst

+
+      "workers": ["worker[01-03]"]
+
+    is same as


is the same as

kokosing · 2017-03-10T05:52:43Z

@yuokada There is a unit test failure on travis

yuokada · 2017-03-10T09:10:53Z

Update doc.

yuokada · 2017-03-10T14:59:03Z

prestoadmin/standalone/config.py

+    if match is not None and len(match.groups()) == 4:
+        prefix, start, end, suffix = match.groups()
+        if int(start) > int(end):
+            raise ValueError("the range format must be ascending order")


@rschlussel Is the error message is OK?

How about "the range must be in ascending order"

rschlussel-zz

slight change in the error message, but otherwise looks good. Thanks!

rschlussel-zz · 2017-03-16T19:30:59Z

prestoadmin/standalone/config.py

+    if match is not None and len(match.groups()) == 4:
+        prefix, start, end, suffix = match.groups()
+        if int(start) > int(end):
+            raise ValueError("the range format must be ascending order")


How about "the range must be in ascending order"

rschlussel-zz · 2017-03-16T21:07:38Z

I made the minor change and merged. Thanks!

yuokada changed the title ~~support range format in worker~~ [WIP] support range format in worker Mar 4, 2017

kokosing suggested changes Mar 6, 2017

View reviewed changes

kokosing previously approved these changes Mar 8, 2017

View reviewed changes

yuokada changed the title ~~[WIP] support range format in worker~~ support range format in worker Mar 8, 2017

kokosing approved these changes Mar 9, 2017

View reviewed changes

rschlussel-zz suggested changes Mar 9, 2017

View reviewed changes

yuokada added 2 commits March 10, 2017 18:12

Add expand_host func

9c05902

Add document for range format

80b379e

yuokada commented Mar 10, 2017

View reviewed changes

rschlussel-zz reviewed Mar 16, 2017

View reviewed changes

rschlussel-zz approved these changes Mar 16, 2017

View reviewed changes

rschlussel-zz closed this Mar 16, 2017

		@@ -140,6 +143,24 @@ def validate_workers(workers):
		return workers


		def expand_host(host):

		self.assertEqual(expected, _expand_host(input_host))


		if __name__ == "__main__":

		@@ -140,6 +143,24 @@ def validate_workers(workers):
		return workers


		def _expand_host(host):

support range format in worker #325

support range format in worker #325

Conversation

yuokada commented Mar 4, 2017 • edited Loading

kokosing commented Mar 4, 2017 via email

kokosing commented Mar 4, 2017 via email

yuokada commented Mar 5, 2017

Choose a reason for hiding this comment

yuokada Mar 7, 2017 • edited Loading

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

kokosing commented Mar 7, 2017

yuokada commented Mar 7, 2017

kokosing commented Mar 7, 2017

kokosing left a 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

yuokada commented Mar 8, 2017

yuokada commented Mar 8, 2017 • edited Loading

kokosing left a 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

rschlussel-zz left a 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

kokosing commented Mar 10, 2017

yuokada commented Mar 10, 2017 • edited Loading

Update doc.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschlussel-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschlussel-zz commented Mar 16, 2017

yuokada commented Mar 4, 2017 •

edited

Loading

yuokada Mar 7, 2017 •

edited

Loading

yuokada commented Mar 8, 2017 •

edited

Loading

yuokada commented Mar 10, 2017 •

edited

Loading