{"action":"created","issue":{"url":"https://api.github.com/repos/criteo/biggraphite/issues/37","repository_url":"https://api.github.com/repos/criteo/biggraphite","labels_url":"https://api.github.com/repos/criteo/biggraphite/issues/37/labels{/name}","comments_url":"https://api.github.com/repos/criteo/biggraphite/issues/37/comments","events_url":"https://api.github.com/repos/criteo/biggraphite/issues/37/events","html_url":"https://github.com/criteo/biggraphite/pull/37","id":162127210,"number":37,"title":"Add a script in utils/ to compute diff between two graphite clusters","user":{"login":"Thib17","id":13062201,"avatar_url":"https://avatars.githubusercontent.com/u/13062201?v=3","gravatar_id":"","url":"https://api.github.com/users/Thib17","html_url":"https://github.com/Thib17","followers_url":"https://api.github.com/users/Thib17/followers","following_url":"https://api.github.com/users/Thib17/following{/other_user}","gists_url":"https://api.github.com/users/Thib17/gists{/gist_id}","starred_url":"https://api.github.com/users/Thib17/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Thib17/subscriptions","organizations_url":"https://api.github.com/users/Thib17/orgs","repos_url":"https://api.github.com/users/Thib17/repos","events_url":"https://api.github.com/users/Thib17/events{/privacy}","received_events_url":"https://api.github.com/users/Thib17/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"unbrice","id":530291,"avatar_url":"https://avatars.githubusercontent.com/u/530291?v=3","gravatar_id":"","url":"https://api.github.com/users/unbrice","html_url":"https://github.com/unbrice","followers_url":"https://api.github.com/users/unbrice/followers","following_url":"https://api.github.com/users/unbrice/following{/other_user}","gists_url":"https://api.github.com/users/unbrice/gists{/gist_id}","starred_url":"https://api.github.com/users/unbrice/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/unbrice/subscriptions","organizations_url":"https://api.github.com/users/unbrice/orgs","repos_url":"https://api.github.com/users/unbrice/repos","events_url":"https://api.github.com/users/unbrice/events{/privacy}","received_events_url":"https://api.github.com/users/unbrice/received_events","type":"User","site_admin":false},"assignees":[{"login":"unbrice","id":530291,"avatar_url":"https://avatars.githubusercontent.com/u/530291?v=3","gravatar_id":"","url":"https://api.github.com/users/unbrice","html_url":"https://github.com/unbrice","followers_url":"https://api.github.com/users/unbrice/followers","following_url":"https://api.github.com/users/unbrice/following{/other_user}","gists_url":"https://api.github.com/users/unbrice/gists{/gist_id}","starred_url":"https://api.github.com/users/unbrice/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/unbrice/subscriptions","organizations_url":"https://api.github.com/users/unbrice/orgs","repos_url":"https://api.github.com/users/unbrice/repos","events_url":"https://api.github.com/users/unbrice/events{/privacy}","received_events_url":"https://api.github.com/users/unbrice/received_events","type":"User","site_admin":false}],"milestone":null,"comments":5,"created_at":"2016-06-24T11:22:44Z","updated_at":"2016-06-27T12:14:58Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/criteo/biggraphite/pulls/37","html_url":"https://github.com/criteo/biggraphite/pull/37","diff_url":"https://github.com/criteo/biggraphite/pull/37.diff","patch_url":"https://github.com/criteo/biggraphite/pull/37.patch"},"body":"\n\n<!-- Reviewable:start -->\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/criteo/biggraphite/37)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/criteo/biggraphite/issues/comments/228728939","html_url":"https://github.com/criteo/biggraphite/pull/37#issuecomment-228728939","issue_url":"https://api.github.com/repos/criteo/biggraphite/issues/37","id":228728939,"user":{"login":"unbrice","id":530291,"avatar_url":"https://avatars.githubusercontent.com/u/530291?v=3","gravatar_id":"","url":"https://api.github.com/users/unbrice","html_url":"https://github.com/unbrice","followers_url":"https://api.github.com/users/unbrice/followers","following_url":"https://api.github.com/users/unbrice/following{/other_user}","gists_url":"https://api.github.com/users/unbrice/gists{/gist_id}","starred_url":"https://api.github.com/users/unbrice/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/unbrice/subscriptions","organizations_url":"https://api.github.com/users/unbrice/orgs","repos_url":"https://api.github.com/users/unbrice/repos","events_url":"https://api.github.com/users/unbrice/events{/privacy}","received_events_url":"https://api.github.com/users/unbrice/received_events","type":"User","site_admin":false},"created_at":"2016-06-27T12:14:58Z","updated_at":"2016-06-27T12:14:58Z","body":"\n\n\n\nReviewed 1 of 2 files at r1, 1 of 1 files at r2.\nReview status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks broke.\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 59 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL19hXSpd-3qG_RnQFY:-KLGdfTfM6wLKK-7hvbz:-1748223957) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L59)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> Done.\n</details>\nactually, ts_to_val = {ts: val for val, ts in json_object['datapoints']}\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 119 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1RCSAJatzndMiEtxC:-KLGm-Rvdf5MO36EM5I0:705650802) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L119)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> Done.\n</details>\nPlease use a consistent definition of the the function through code, preferably by refering to the ABC from subclasses.\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 169 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1RfROR4HFha5WFFV5:-KLGfCf0InNGGfNCFJj7:1858798218) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L169)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> No, because both clusters are working the same way, so we need to specify that there is no difference. And by returning [0.0], all dis-symmetry pctls will be set at 0% as we want. (in the other case, [] lead to dis-symmetry pctls set at 100%)\n</details>\nSo you are changing the return definition to simplify display code at the expense of intelligibility? If so, it is not the most wise of tradeoffs.\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 196 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1Rt1naHkWMLSASIYm:-KLGfd5ie2WVqWq76Se8:1386855955) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L196)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> Done.\n</details>\nAm I right thinking that the comment is only true because of what you know of how is the call called rather than because of its definition? If so please reduce coupling by returning None instead of something that looks good on the UI.\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 214 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1MzySnGJHqJS0yLOW:-KLGfydnfOBu6_CUwuAC:979714289) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L214)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> Done.\n</details>\nBut you didn't make it an ABC\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 240 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1NOPjPnJdyw2z52kc:-KLGg1JNOyilupdAHs6r:547208084) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L240)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> Done.\n</details>\nNot pushed?\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 262 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1OPmXF4yvchvwEaOT:-KLGg71aWZEUUhJZc0_N:-395260970) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L262)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> Done.\n</details>\nuse of iteritems not pushed?\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 325 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1OUn0vUwXok0mB5Lg:-KLGgLbA5h6TsDkuGyk1:1534397955) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L325)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> No, but print_header() should be rename as print_parameters()\n</details>\nOK, feel free to do both changes\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 408 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1UWCC9_Px8Lyr2Xjr:-KLGgr_Jf5qRePKTbLrq:-44062057) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L408)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> I don't understand what you say\n</details>\nsorry, I meant to say: how are the two branches different?\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 426 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1W5rzAMf-9FyW3k9N:-KLGjK5Cb--b5evALKSa:447947995) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/clusters_diff.py#L426)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> usage: clusters_diff.py [-h] --hosts HOSTS HOSTS -u USERNAME\n>                         (-p PASSWORDS PASSWORDS | -k AUTH_KEYS AUTH_KEYS)\n>                         --input_filename INPUT_FILENAME [--From FROM]\n>                         [--Until UNTIL] [--timeout_s TIMEOUT_S]\n>                         [--threshold THRESHOLD] [--printdiff | --no-printdiff]\n>                         [--timing | --no-timing] [-v] [--show_max SHOW_MAX]\n> \n> Compare two Graphite clusters for a given list of queries.\n> \n> optional arguments:\n>   -h, --help            show this help message and exit\n>   --hosts HOSTS HOSTS   Cluster hostnames\n>   -u USERNAME, --username USERNAME\n>                         Username\n>   -p PASSWORDS PASSWORDS, --passwords PASSWORDS PASSWORDS\n>                         Passwords\n>   -k AUTH_KEYS AUTH_KEYS, --auth_keys AUTH_KEYS AUTH_KEYS\n>                         Hosts keys\n>   --input_filename INPUT_FILENAME\n>                         Text file containing queries\n>   --From FROM           From\n>   --Until UNTIL         Until\n>   --timeout_s TIMEOUT_S\n>                         Timeout in seconds to fetch queries\n>   --threshold THRESHOLD\n>                         Relative threshold for equality between two value\n>   --printdiff           Print the diff returned by datadiff module\n>   --no-printdiff        Do not print the diff returned by datadiff module\n>   --timing              Print timing information\n>   --no-timing           Do not print timing information\n>   -v, --verbosity\n>   --show_max SHOW_MAX   Truncate the number of shown percentiles\n</details>\nThanks! Few remarks then\n- Please refrain from using the redundant \"--show_max SHOW-MAX\" style: each of the component of --help should have a purpose in conveying the usage of your software in a concise way.\n- I don't think that a reasonably smart user can figure out what \"printdiff\" and \"show_max\" do from their descriptions (feel free to show this to a colleague and check)\n- did you consider using the user's .netrc file, for passwords? It would be more secure and it is the \"standard way\" on unix\n- you use plural for thinks like `HOSTS` that are singular (`HOST` is a single host, `--hosts` is the list)\n- you use inconsistent case eg: `--From`\n- command line generally uses - not _ as a word separator \n- `--no-*` is redundant, please only keep the non-default option\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 161 \\[r2\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KLGeFnU4yN3cbo08pXt:-KLGeFnU4yN3cbo08pXu:-1558369097) ([raw file](https://github.com/criteo/biggraphite/blob/dc6f234c05cdd663f78273af22414e0b4efdb47b/biggraphite/cli/clusters_diff.py#L161)):*\n> ```Python\n>             return 0.0\n> \n>         count = 0.0\n> ```\n\nI am not sure incrementing floats is safe for this value range (for a big f, f+1 == f), I suggest using ints instead and return float(count)/len(val_dissimetries)\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 296 \\[r2\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KLGgEgxdRdRl8Krrqyf:-KLGgEgxdRdRl8Krrqyg:-1289192747) ([raw file](https://github.com/criteo/biggraphite/blob/dc6f234c05cdd663f78273af22414e0b4efdb47b/biggraphite/cli/clusters_diff.py#L296)):*\n> ```Python\n>         del query_percentiles_list[show_max:]\n> \n>         self._print(\"\\n%s for : \" % (Printer.QUERY_PCTLS_DESCRIPTION))\n> ```\n\nextra paren\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 301 \\[r2\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KLGgG30HujWP1QCRpPB:-KLGgG30HujWP1QCRpPC:-1289192747) ([raw file](https://github.com/criteo/biggraphite/blob/dc6f234c05cdd663f78273af22414e0b4efdb47b/biggraphite/cli/clusters_diff.py#L301)):*\n> ```Python\n> \n>             if verbosity >= 1:\n>                 self._print(\"\\n\\t%s for : \" % (Printer.TARGET_PCTLS_DESCRIPTION))\n> ```\n\nextra paren\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 382 \\[r2\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KLGgZH_alAs3TdJgU58:-KLGgZH_alAs3TdJgU59:-1451447800) ([raw file](https://github.com/criteo/biggraphite/blob/dc6f234c05cdd663f78273af22414e0b4efdb47b/biggraphite/cli/clusters_diff.py#L382)):*\n> ```Python\n> \n> def fetch_queries(host, auth_key, queries,\n>                   from_param, until_param, timeout_s, threshold, pbar_update):\n> ```\n\nPlease avoid pbar_update as it couples the two concepts, suggest progress_cb\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 427 \\[r2\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KLGiBl6vXNouO3fmPWt:-KLGiBl6vXNouO3fmPWu:-2054153690) ([raw file](https://github.com/criteo/biggraphite/blob/dc6f234c05cdd663f78273af22414e0b4efdb47b/biggraphite/cli/clusters_diff.py#L427)):*\n> ```Python\n> \n>     names = {a.name for a in diffables_a}\n>     names.union({b.name for b in diffables_b})\n> ```\n\nunion doesn't mutate, what you're probably looking for is called updated and can take a list (cheaper)\n\n---\n\n*[biggraphite/cli/interface_clusters_diff.py, line 1 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/37#-KL1OhWB_2hAwCe1uKZ8:-KLGiVLRQ7LooMEbudnB:547208084) ([raw file](https://github.com/criteo/biggraphite/blob/cabc4bac652e3f485d98a244d5bd74ec225e49f5/biggraphite/cli/interface_clusters_diff.py#L1)):*\n<details><summary><i>Previously, Thib17 wrote…</i></summary>\n> Done.\n</details>\nNot pushed?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/criteo/biggraphite/37)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"unbrice"}}