{"action":"created","issue":{"url":"https://api.github.com/repos/youtube/vitess/issues/1907","repository_url":"https://api.github.com/repos/youtube/vitess","labels_url":"https://api.github.com/repos/youtube/vitess/issues/1907/labels{/name}","comments_url":"https://api.github.com/repos/youtube/vitess/issues/1907/comments","events_url":"https://api.github.com/repos/youtube/vitess/issues/1907/events","html_url":"https://github.com/youtube/vitess/pull/1907","id":168382202,"number":1907,"title":"Various Healthcheck and discovery / gateway improvements.","user":{"login":"alainjobart","id":4625714,"avatar_url":"https://avatars.githubusercontent.com/u/4625714?v=3","gravatar_id":"","url":"https://api.github.com/users/alainjobart","html_url":"https://github.com/alainjobart","followers_url":"https://api.github.com/users/alainjobart/followers","following_url":"https://api.github.com/users/alainjobart/following{/other_user}","gists_url":"https://api.github.com/users/alainjobart/gists{/gist_id}","starred_url":"https://api.github.com/users/alainjobart/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alainjobart/subscriptions","organizations_url":"https://api.github.com/users/alainjobart/orgs","repos_url":"https://api.github.com/users/alainjobart/repos","events_url":"https://api.github.com/users/alainjobart/events{/privacy}","received_events_url":"https://api.github.com/users/alainjobart/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/youtube/vitess/labels/cla:%20yes","name":"cla: yes","color":"009800"}],"state":"open","locked":false,"assignee":{"login":"michael-berlin","id":3919251,"avatar_url":"https://avatars.githubusercontent.com/u/3919251?v=3","gravatar_id":"","url":"https://api.github.com/users/michael-berlin","html_url":"https://github.com/michael-berlin","followers_url":"https://api.github.com/users/michael-berlin/followers","following_url":"https://api.github.com/users/michael-berlin/following{/other_user}","gists_url":"https://api.github.com/users/michael-berlin/gists{/gist_id}","starred_url":"https://api.github.com/users/michael-berlin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/michael-berlin/subscriptions","organizations_url":"https://api.github.com/users/michael-berlin/orgs","repos_url":"https://api.github.com/users/michael-berlin/repos","events_url":"https://api.github.com/users/michael-berlin/events{/privacy}","received_events_url":"https://api.github.com/users/michael-berlin/received_events","type":"User","site_admin":false},"assignees":[{"login":"michael-berlin","id":3919251,"avatar_url":"https://avatars.githubusercontent.com/u/3919251?v=3","gravatar_id":"","url":"https://api.github.com/users/michael-berlin","html_url":"https://github.com/michael-berlin","followers_url":"https://api.github.com/users/michael-berlin/followers","following_url":"https://api.github.com/users/michael-berlin/following{/other_user}","gists_url":"https://api.github.com/users/michael-berlin/gists{/gist_id}","starred_url":"https://api.github.com/users/michael-berlin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/michael-berlin/subscriptions","organizations_url":"https://api.github.com/users/michael-berlin/orgs","repos_url":"https://api.github.com/users/michael-berlin/repos","events_url":"https://api.github.com/users/michael-berlin/events{/privacy}","received_events_url":"https://api.github.com/users/michael-berlin/received_events","type":"User","site_admin":false}],"milestone":null,"comments":4,"created_at":"2016-07-29T18:12:05Z","updated_at":"2016-08-02T22:46:41Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/youtube/vitess/pulls/1907","html_url":"https://github.com/youtube/vitess/pull/1907","diff_url":"https://github.com/youtube/vitess/pull/1907.diff","patch_url":"https://github.com/youtube/vitess/pull/1907.patch"},"body":"This flag allows sending a Up=false event to the HealthCHeck Listener\r\nwhen a tablet changes type. If the listener indexes its tablet by type,\r\nit is useful.\r\n\r\nFixing the vtctld listener to correctly handle the event.\r\n\r\n@michael-berlin @pkulsh the vtctld listener should be more correct now, it should stay in sync and remove tablets that are gone.\r\n\r\n(note we need the flag as the internal Listener implementation does not need the extra event).\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/youtube/vitess/1907)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/youtube/vitess/issues/comments/237071971","html_url":"https://github.com/youtube/vitess/pull/1907#issuecomment-237071971","issue_url":"https://api.github.com/repos/youtube/vitess/issues/1907","id":237071971,"user":{"login":"alainjobart","id":4625714,"avatar_url":"https://avatars.githubusercontent.com/u/4625714?v=3","gravatar_id":"","url":"https://api.github.com/users/alainjobart","html_url":"https://github.com/alainjobart","followers_url":"https://api.github.com/users/alainjobart/followers","following_url":"https://api.github.com/users/alainjobart/following{/other_user}","gists_url":"https://api.github.com/users/alainjobart/gists{/gist_id}","starred_url":"https://api.github.com/users/alainjobart/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alainjobart/subscriptions","organizations_url":"https://api.github.com/users/alainjobart/orgs","repos_url":"https://api.github.com/users/alainjobart/repos","events_url":"https://api.github.com/users/alainjobart/events{/privacy}","received_events_url":"https://api.github.com/users/alainjobart/received_events","type":"User","site_admin":false},"created_at":"2016-08-02T22:46:40Z","updated_at":"2016-08-02T22:46:40Z","body":"I agree on the RPC Name, I changed the comments to StreamHealth.\n\nwait_for_drain_test.go we might need to talk about it in person, faster.\n\n---\n\nReview status: all files reviewed at latest revision, 32 unresolved discussions.\n\n---\n\n*[go/vt/discovery/fake_healthcheck.go, line 54 \\[r5\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBDg8yd3ZX8P5XBASq:-KOCH2fDYwVpxdd_f--f:-499146009) ([raw file](https://github.com/youtube/vitess/blob/5beb4caa10ed46b507497f6b3cd993bc86007b59/go/vt/discovery/fake_healthcheck.go#L54)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Does the default implementation call the listener during `AddTablet` as well? I assume so?\n> \n> Then it's maybe not necessary to point this out? \n> If it doesn't, the fake shouldn't do it as well.\n> \n> If you want to keep the comment, make this part of the first sentence instead?\n> \n> `// AddTablet adds the tablet and calls the listener.`\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/fake_healthcheck.go, line 140 \\[r5\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBEFTICKh3WgJehmr5:-KOCH8IS519zKCg1XpB4:449837377) ([raw file](https://github.com/youtube/vitess/blob/5beb4caa10ed46b507497f6b3cd993bc86007b59/go/vt/discovery/fake_healthcheck.go#L140)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Same comment as above.\n</details>\nthis one it needs to be explicit, as it is an helper function for tests. So made it clearer.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 823 \\[r3\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KO4S3NByaoqIJRfFxMd:-KOCJpaKjD4wvb7UicHO:1519836834) ([raw file](https://github.com/youtube/vitess/blob/5db6dd08ba92949d254505c5e903651a3002ca1e/go/vt/discovery/healthcheck.go#L823)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> I'm sorry but these are the rules :)\n> \n> https://github.com/golang/go/wiki/CodeReviewComments#receiver-names\n> \n> The other thing to keep in mind is that abbreviations are not helpful at all in my opinion. They require to much work to translate them back to what you mean.\n</details>\nMy main objection is for searching. Impossible to search a one letter variable name. tsc was never anywhere else in this source file.\n\nBut hey, that's the rules, renamed it to tc.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 978 \\[r5\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBEN3zpB1W09T0iRqN:-KOCKNYZn-stOVunO6E_:-499146009) ([raw file](https://github.com/youtube/vitess/blob/5beb4caa10ed46b507497f6b3cd993bc86007b59/go/vt/discovery/healthcheck.go#L978)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> nit: Missing period.\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 979 \\[r5\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBEY5ME4kk26fwtiZr:-KOCKOEItUMSj-4NI8MO:-499146009) ([raw file](https://github.com/youtube/vitess/blob/5beb4caa10ed46b507497f6b3cd993bc86007b59/go/vt/discovery/healthcheck.go#L979)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Name it `ResetForTesting` instead to clarify that it should be used only by tests?\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 95 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBY6w_3-zWuO7AAWYW:-KOCHTgK9amy-bgQIbqW:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L95)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> s/type/its type/\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 98 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBYCUul-eNaNRb87SH:-KOCHUG4vAowQ-ovCgXa:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L98)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Indent this line.\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 107 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBYv4-ukmYFKRNCvIO:-KOCHbaw8zyeLjtgUI_5:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L107)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Please change `job` with something else as its overloaded with our internal definition. That is, a job is a collection of tasks (which you also express by having `+task` further done.)\n> \n> So maybe just change it to `task`?\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 109 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBZOadyvLhSoUger89:-KOCHl3ZhzxAnGposWCP:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L109)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> s/replica/tabletType/ ?\n> \n> Instead of task, you could say `taskIndex` here to better distinguish between the task instance and its unique identifier among the tasks of a job.\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 112 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBZhg7xLw21WVD9lz0:-KOCHxcPxXV_kEWs9xeX:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L112)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> \"healthcheck RPC\" to clarify what healthcheck refers to in this case.\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 115 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBYLolvswflgO6OsPM:-KOCI-Ivx4cGefQy_PKz:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L115)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Remove ` //` here.\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 119 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBZz_OvZZBhVKh38i8:-KOCIEtns7_Tg0_exKnP:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L119)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> \"was the master\" is not true?\n> \n> It's actually the last time this tablet was elected as master?\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 122 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOB_a0lgJ_ZR0cq48B0:-KOCIQd-ChQiz5mhLJWV:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L122)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> maybe clarify this e.g.\n> \n> `Stats has the current health status (e.g. the tablet's replication lag).`\n> \n> ?\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 135 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBa3z_A7RaeTQHNv4e:-KOCIgQerady0kTVBvUV:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L135)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> > HealthCheck\n> \n> Unrelated to this change. Can you please change this to \"healthcheck\" as its misleading in the current form.\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 140 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBaPfmCHuQPrs-oZ3x:-KOCIyXsdQ9v7u2l0Gsf:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L140)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> I suggest to rephrase this and clarify it.\n> \n> For example:\n> \n> ```go\n> // Updates to the health of a registered tablet can be watched by registering a listener.\n> // To get the underlying \"TabletConn\" object which is used for each tablet, use the \"GetConnection()\" method below and pass in the \"Tablet\" struct which is also sent to the listener in each update.\n> ```\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 158 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBcTVvgX25sRlJn-ge:-KOCJ2lzLpzkN_oT_v7e:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L158)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> \"like a job or instance name\"\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck.go, line 160 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBcLTbT3xfeUJFFJNg:-KOCJAXbp1qGCQWsmVe3:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck.go#L160)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> s/health check on it/streaming healthcheck RPC to it/\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck_wait.go, line 29 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBuwPQIT1E2Pw51na6:-KOCL63qhgeN6cqEP6mC:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck_wait.go#L29)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Since this is part of TabletStatsCache now:\n> \n> Can you please move it into `tablet_stats_cache.go` or rename this file to e.g. `tablet_stats_cache_wait.go`?\n> \n> Otherwise the current filename is confusing.\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck_wait.go, line 101 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBv_5_nc_0Uh5PLuP5:-KOCLHBWwCZlShICISvp:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck_wait.go#L101)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> The variable name is difficult to read: is it `tl` or `t1`?\n> \n> You can use something more descriptive instead e.g. `stats` or `tabletStats`?\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/healthcheck_wait_test.go, line 1 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBwPS5hgWg7AvTE5tu:-KOCLIMIuucf4U4FyriZ:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/healthcheck_wait_test.go#L1)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> This file should be renamed as well.\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/tablet_stats_cache.go, line 152 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOC280GYLzxvxpTf_HI:-KOCLkn20VfJuONmXw02:818005152) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/discovery/tablet_stats_cache.go#L152)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Please write a comment here and at `GetHealthyTabletStats` that a call for `TabletType_MASTER` will always return one entry at most.\n</details>\nActually this is only true for the Healthy one, this one can return more than one. Updated the comment there.\n\n---\n\n*[go/vt/discovery/tablet_stats_cache_test.go, line 13 \\[r7\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBQP7xGdF5FHqibliL:-KOCLywxk-P4FIP811w0:-499146009) ([raw file](https://github.com/youtube/vitess/blob/909621a3d0a028fc4f7b8aa2df576d841658c3ab/go/vt/discovery/tablet_stats_cache_test.go#L13)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> \"functionality\" (one n too much)\n> \n> \"TabletsStatsCache class\" (without \"Test\" prefix)\n</details>\nDone.\n\n---\n\n*[go/vt/discovery/tablet_stats_cache_test.go, line 15 \\[r7\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBQfR_VHz8vg1bH75p:-KOCMKnlHzEfNpdhli9b:-499146009) ([raw file](https://github.com/youtube/vitess/blob/909621a3d0a028fc4f7b8aa2df576d841658c3ab/go/vt/discovery/tablet_stats_cache_test.go#L15)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Can you please rephrase this comment?\n> \n> It sounds like you want to say that you're testing this class without registering it as listener at a HealthCheck instance?\n> \n> I would phrase it along these lines:\n> \n> ```go\n> // Unit test TabletStatsCache and emulate getting called by an HealthCheck instance by calling \"StatsUpdate\" directly in the test.\n> ```\n</details>\nDone.\n\n---\n\n*[go/vt/tabletmanager/binlog_players.go, line 72 \\[r8\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBRsTplrpbMzIre7FS:-KOCMfirb2wRjoC_xHCx:563994719) ([raw file](https://github.com/youtube/vitess/blob/8e594a60da2237785bcebdf1293f20afcc025e2b/go/vt/tabletmanager/binlog_players.go#L72)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> > healthcheck\n> \n> Let's use the original capitalization if we refer to a field or a class.\n> \n> In this you probably meant `healthCheck` to refer to the field?\n> \n> I'm also not happy with `tsc`, especially since the other instances are not abbreviated and instead named `healthCheck` and `shardReplicationWatcher`.\n</details>\nRenamed to tabletStatsCache.\n\n---\n\n*[go/vt/tabletmanager/binlog_players.go, line 307 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBgwa7hdTgCFWGlh_O:-KOCMwuzfpDYqXbsDSKn:-499146009) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/tabletmanager/binlog_players.go#L307)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Maybe rephrase this and say instead:\n> \n> `// Note: We cannot use tsc.GetHealthyTabletStats() here because it does not return non-serving tablets. However, we might replicate from these as well.`\n</details>\nDone.\n\n---\n\n*[go/vt/vtctld/tablet_stats_cache.go, line 65 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBeBubGiuOxGG4Daqy:-KOCN-M5Aw4S48U0Sxr8:160841956) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/vtctld/tablet_stats_cache.go#L65)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> FYI: This is not necessary when we actually use the struct e.g. it's passed in to `HealthCheck.SetListener()`.\n</details>\nI agree, but then the error may happen later in an unrelated file. I like this better as it will check exactly in this module.\n\n---\n\n*[go/vt/vtgate/scatter_conn_test.go, line 33 \\[r5\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBExFHauJHspEol1EE:-KOCNSxyoqSvzPzbpmss:-455557932) ([raw file](https://github.com/youtube/vitess/blob/5beb4caa10ed46b507497f6b3cd993bc86007b59/go/vt/vtgate/scatter_conn_test.go#L33)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> optional: This should be named `sc`?\n</details>\nsure.\n\n---\n\n*[go/vt/vtgate/gateway/discoverygateway.go, line 109 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBx_pwJpAB0r4-Pban:-KOCNYGcusPIYZ48o3iV:-35680544) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/vtgate/gateway/discoverygateway.go#L109)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> optional: With the new `if` above, this became dead code. You could delete this. I'm fine with keeping it.\n</details>\nThis is part of the interface, it could be called by itself, not just from above. So I think it's worth keeping.\n\n---\n\n*[go/vt/vtgate/gateway/discoverygateway_test.go, line 121 \\[r5\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBOqzJeARw6svcKcAa:-KOCNmZcKBbIBg-rFk-c:519114425) ([raw file](https://github.com/youtube/vitess/blob/ccb2f11b2aae33bfc7250524d176764135a1c602/go/vt/vtgate/gateway/discoverygateway_test.go#L121)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Do we use substantial test coverage by deleting this code?\n> \n> It's hard to tell for me at this point. Deleting it is probably fine?\n</details>\nIt's hard for me to tell too, I'll look into it later. It was part of the original non-generic tests, I am not 100% sure I replaced them all.\n\n---\n\n*[go/vt/vtgate/gateway/gateway.go, line 82 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOByT--qQmvO4my8g_I:-KOCOCA_ZcOYu5YeAkMZ:-1650631671) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/vtgate/gateway/gateway.go#L82)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> This comment is hard to understand.\n> \n> Can you please break it down into multiple shorter sentences?\n> \n> After all, is it actually worth to keep the comment? You're mainly explaining the reason why this is a separate function? Isn't the main reason that different implementations have different implementations of this method?\n</details>\nThe comment tries to address why it is a separate method. But I'm gonna rework it eventually, this is just not right. It's too hard however for me to reason about this just now, before having done the google internal implementation as well. More on this later.\n\n---\n\n*[go/vt/worker/split_clone.go, line 611 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOC0Uluqz74ZkYRObI9:-KOCOaNkhTajCxs1dCMZ:1623381846) ([raw file](https://github.com/youtube/vitess/blob/ccb2f11b2aae33bfc7250524d176764135a1c602/go/vt/worker/split_clone.go#L611)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> optional: I had these function calls on a new line on purpose to make it easier to read :P\n</details>\nI thought that made it more confusing... :)\n\n---\n\n*[go/vt/worker/topo_utils.go, line 36 \\[r18\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOC17YFcCi3eeOp93Jw:-KOCPBBV0uhNGkAgiffA:1330586045) ([raw file](https://github.com/youtube/vitess/blob/b57b2a47aad634ff03aa26e29d31863b15c52e35/go/vt/worker/topo_utils.go#L36)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> Here should be an assertion that `tsc` must be `nil` as well.\n</details>\nActually I meant to remove healthcheck as a parameter, but forgot. So did that as well.\n\n---\n\n*[go/vt/wrangler/testlib/wait_for_drain_test.go, line 56 \\[r11\\]](https://reviewable.io:443/reviews/youtube/vitess/1907#-KOBWDaEgamwhFW_QLpq:-KOCPujypMLd1MrxrXWU:-1796478003) ([raw file](https://github.com/youtube/vitess/blob/789aaf3fbf65da2b91e2022b7b3f6f5a9d090d27/go/vt/wrangler/testlib/wait_for_drain_test.go#L56)):*\n<details><summary><i>Previously, michael-berlin (Michael Berlin) wrote…</i></summary>\n> What was the problem you did see?\n> \n> Since this timeout is higher than the \"timeout\" value below, you may be changing the intent of the test and also the coverage.\n> \n> The WaitForDrain code has a `time.Sleep(healthCheckTimeout)` in `keyspace.go`. It's using the timeout value you're changing here.\n> \n> We did this on purpose to make sure that we don't miss any health tablets in the list of tablets to look after.\n> \n> If this timeout is higher than the overall timeout for the vtctl command, we may skip some of the code in the test?\n</details>\nThe problem I saw is that the StreamHealth RPC connection error-ed out, as its context was cancelled (this parameter is also used as healthcheck's timeout to kill a streaming connection it hasn't heard from). So the LastError field was not nil, and that is used by the healthy tablet check. Whereas it is not use by the check you had before, which was wrongly just looking at HealthErr inside the stats.\n\nI am not sure we still test every single case here, but the new behavior is definitely better, we need to look at LastError too.\n\nMaybe this makes the case for splitting both parameters?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/youtube/vitess/1907#-:-KOCQqkDwgsDddsk5ruD:63463311)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"alainjobart"}}