{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7781","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7781/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7781/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7781/events","html_url":"https://github.com/cockroachdb/cockroach/pull/7781","id":165117999,"number":7781,"title":"sql: ExtendLease() returns existing valid unexpired lease","user":{"login":"vivekmenezes","id":11774212,"avatar_url":"https://avatars.githubusercontent.com/u/11774212?v=3","gravatar_id":"","url":"https://api.github.com/users/vivekmenezes","html_url":"https://github.com/vivekmenezes","followers_url":"https://api.github.com/users/vivekmenezes/followers","following_url":"https://api.github.com/users/vivekmenezes/following{/other_user}","gists_url":"https://api.github.com/users/vivekmenezes/gists{/gist_id}","starred_url":"https://api.github.com/users/vivekmenezes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vivekmenezes/subscriptions","organizations_url":"https://api.github.com/users/vivekmenezes/orgs","repos_url":"https://api.github.com/users/vivekmenezes/repos","events_url":"https://api.github.com/users/vivekmenezes/events{/privacy}","received_events_url":"https://api.github.com/users/vivekmenezes/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"andreimatei","id":377201,"avatar_url":"https://avatars.githubusercontent.com/u/377201?v=3","gravatar_id":"","url":"https://api.github.com/users/andreimatei","html_url":"https://github.com/andreimatei","followers_url":"https://api.github.com/users/andreimatei/followers","following_url":"https://api.github.com/users/andreimatei/following{/other_user}","gists_url":"https://api.github.com/users/andreimatei/gists{/gist_id}","starred_url":"https://api.github.com/users/andreimatei/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andreimatei/subscriptions","organizations_url":"https://api.github.com/users/andreimatei/orgs","repos_url":"https://api.github.com/users/andreimatei/repos","events_url":"https://api.github.com/users/andreimatei/events{/privacy}","received_events_url":"https://api.github.com/users/andreimatei/received_events","type":"User","site_admin":false},"assignees":[{"login":"andreimatei","id":377201,"avatar_url":"https://avatars.githubusercontent.com/u/377201?v=3","gravatar_id":"","url":"https://api.github.com/users/andreimatei","html_url":"https://github.com/andreimatei","followers_url":"https://api.github.com/users/andreimatei/followers","following_url":"https://api.github.com/users/andreimatei/following{/other_user}","gists_url":"https://api.github.com/users/andreimatei/gists{/gist_id}","starred_url":"https://api.github.com/users/andreimatei/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andreimatei/subscriptions","organizations_url":"https://api.github.com/users/andreimatei/orgs","repos_url":"https://api.github.com/users/andreimatei/repos","events_url":"https://api.github.com/users/andreimatei/events{/privacy}","received_events_url":"https://api.github.com/users/andreimatei/received_events","type":"User","site_admin":false}],"milestone":null,"comments":1,"created_at":"2016-07-12T16:13:00Z","updated_at":"2016-07-13T16:29:15Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/7781","html_url":"https://github.com/cockroachdb/cockroach/pull/7781","diff_url":"https://github.com/cockroachdb/cockroach/pull/7781.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/7781.patch"},"body":"ExtendLease() was always writing to the table descriptor resulting\r\nin the system config getting gossiped more often than needed.\r\nThis change writes a lease with a new expiration time only when the\r\nlease is closer to its expiration time. With schema changes now\r\nexecuting in chunks and ExtendLease() being called more often after\r\neach chunk is processed, forcing a new expiration time is\r\nunnecessary.\r\n\r\nfixes #7724\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/cockroachdb/cockroach/7781)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/232410427","html_url":"https://github.com/cockroachdb/cockroach/pull/7781#issuecomment-232410427","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7781","id":232410427,"user":{"login":"andreimatei","id":377201,"avatar_url":"https://avatars.githubusercontent.com/u/377201?v=3","gravatar_id":"","url":"https://api.github.com/users/andreimatei","html_url":"https://github.com/andreimatei","followers_url":"https://api.github.com/users/andreimatei/followers","following_url":"https://api.github.com/users/andreimatei/following{/other_user}","gists_url":"https://api.github.com/users/andreimatei/gists{/gist_id}","starred_url":"https://api.github.com/users/andreimatei/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andreimatei/subscriptions","organizations_url":"https://api.github.com/users/andreimatei/orgs","repos_url":"https://api.github.com/users/andreimatei/repos","events_url":"https://api.github.com/users/andreimatei/events{/privacy}","received_events_url":"https://api.github.com/users/andreimatei/received_events","type":"User","site_admin":false},"created_at":"2016-07-13T16:29:15Z","updated_at":"2016-07-13T16:29:15Z","body":"<img class=\"emoji\" title=\":lgtm:\" alt=\":lgtm:\" align=\"absmiddle\" src=\"https://reviewable.io/lgtm.png\" height=\"20\" width=\"61\"/>\n\n---\n\nReview status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions.\n\n---\n\n*[sql/schema_changer.go, line 81 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KM_2LkF8iUR3c3uZLpY:-KM_2LkF8iUR3c3uZLpZ:1643584658) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer.go#L81)):*\n> ```Go\n> func (sc *SchemaChanger) createSchemaChangeLease() sqlbase.TableDescriptor_SchemaChangeLease {\n> \treturn sqlbase.TableDescriptor_SchemaChangeLease{\n> \t\tNodeID: sc.nodeID, ExpirationTime: timeutil.Now().Add(jitteredLeaseDuration()).UnixNano()}\n> ```\n\nif you feel so inclined, let's change this to not use `LeaseDuration` any more. Let's use a different constant for schema change leases than for descriptor leases.\nAnd I don't think we need jittering here. I think that's only useful for descriptor leases.\n\n---\n\n*[sql/schema_changer.go, line 150 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KM_0QxBmiStwvax_a3K:-KM_0QxC4mQSv4sMz7_w:-1319273077) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer.go#L150)):*\n> ```Go\n> \n> // ExtendLease for the current leaser if necessary.\n> func (sc *SchemaChanger) ExtendLease(\n> ```\n\nrename to `ExtendLeaseMaybe`?\n\n---\n\n*[sql/schema_changer.go, line 150 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KM_0T_Ho1F-zILlAYXA:-KM_0T_Ho1F-zILlAYXB:275326648) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer.go#L150)):*\n> ```Go\n> \n> // ExtendLease for the current leaser if necessary.\n> func (sc *SchemaChanger) ExtendLease(\n> ```\n\nComment that this needs to be called early and often when performing schema changes.\n\n---\n\n*[sql/schema_changer.go, line 161 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KMZvGxK2xpTUFjUgiC0:-KMZvGxK2xpTUFjUgiC1:-1788340267) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer.go#L161)):*\n> ```Go\n> \n> \t\t// Check if there is still time on this lease.\n> \t\tminDesiredExpiration := timeutil.Now().Add(MinLeaseDuration)\n> ```\n\n`MinLeaseDuration` refers to descriptor leases. Let's create a new constant for the other types of leases.\nIt's already confusing enough that the code calls them both \"leases\".\n\n---\n\n*[sql/schema_changer.go, line 161 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KM_0i4UszlY7Siv61UO:-KM_0i4UszlY7Siv61UP:503908197) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer.go#L161)):*\n> ```Go\n> \n> \t\t// Check if there is still time on this lease.\n> \t\tminDesiredExpiration := timeutil.Now().Add(MinLeaseDuration)\n> ```\n\nI'd say let's move this out of the txn, since it's the common case, and the txn reading the descriptor is expensive.\nIf you have a lease that's nowhere close to expiration, no point in reading anything from the database. This would make the guarantees of `ExtendLease` not erroring weaker - we'd no longer be checking that we're extending the most recent lease. To fix that, we can add a `tainted` field to the lease, and whenever one gets a new lease (through this `ExtendsLease`), we'd set the bit on the old one.\n\n---\n\n*[sql/schema_changer_test.go, line 59 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KMZtwM7mAu1Ok1BKpdC:-KMZtwM7mAu1Ok1BKpdD:227701113) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer_test.go#L59)):*\n> ```Go\n> \t// Set MinLeaseDuration to always expire the lease.\n> \tminLeaseDuration := csql.MinLeaseDuration\n> \tcsql.MinLeaseDuration = 2 * csql.LeaseDuration\n> ```\n\nLet's keep fighting the crusade changing such globals :). They lead to races and overall headaches. Pass it through the params.\nIn fact, although setting this to `2 * LeaseDuration` is a nice trick, I didn't notice it and spent a bit thinking about how this test is supposed to work at all given that we're not advancing the clock. Maybe a better thing for this test is to pass a testing knob that forces the always-renew behavior.\n\nIf you keep the lease life value, instead of changing it below (which you couldn't easily do through the `TestServerArgs`), you can just create a new server with a different value.\n\n\nOf course, the correct thing to do is neither changing the value, nor introducing a knob, but advancing the clock, but alas the `TestServer` doesn't take a manual clock. If you feel like rabbit holeing....\n\n---\n\n*[sql/schema_changer_test.go, line 87 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KM_1d0k6fw01lPjmEdR:-KM_1d0k6fw01lPjmEdS:-1368014613) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer_test.go#L87)):*\n> ```Go\n> \n> \t// Acquiring another lease will fail.\n> \tif newLease, err := changer.AcquireLease(); err == nil {\n> ```\n\nuse `testutils.IsError`. It's better and you don't need to write a new massage in `Fatalf`.\n\n---\n\n*[sql/schema_changer_test.go, line 103 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KMZuNmBEe9X_KgW-PkG:-KMZuNmBEe9X_KgW-PkH:1211587664) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer_test.go#L103)):*\n> ```Go\n> \t// The new lease is a brand new lease.\n> \tif newLease == lease {\n> \t\tt.Fatalf(\"leases are equal: %v\", lease)\n> ```\n\nI think a better error would be \"lease was not extended\"\n\n---\n\n*[sql/schema_changer_test.go, line 106 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KMZxAlf6z1YVyaOFf1m:-KMZxAlf6z1YVyaOFf1n:2003023499) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer_test.go#L106)):*\n> ```Go\n> \t}\n> \n> \t// Extending an old lease fails.\n> ```\n\nCan you explain why this is expected to fail?\nAlso, use `testutils.IsError(...)`\n\nI suspect we're using this failure to `ExtendLease()` as a proxy for checking that a schema changer with anything but the latest lease cannot perform mutations. It'd be cool if we'd also test at least one of the schema changers to actually try to do a mutation after a lease has expired, and check that that fails.\n\nMaybe we can also improve the schema changer interface so that it's more stupid-proof (less likely for someone to forget to call ExtendLease). How about we make all the code in schema changer only get table descriptor through a (schema change) lease (so the lease would have a `GetDesc()` method, which would call `findTableWithLease()`). Or in some other way ensure that the lease is always verified to be the latest when mutations are applied. I don't have a great idea.\n\n---\n\n*[sql/schema_changer_test.go, line 112 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-KMZxdLRAs8TI3Q7YX8k:-KMZxdLRAs8TI3Q7YX8l:-1801907613) ([raw file](https://github.com/cockroachdb/cockroach/blob/436bacb25021e095cee8289d9eff7be63634b2ca/sql/schema_changer_test.go#L112)):*\n> ```Go\n> \t}\n> \n> \t// Releasing an old lease fails.\n> ```\n\nis the fact that releasing an old lease fails important, or just an implementation artifact? If it's not important, I'd say remove it from the test.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/7781#-:-KM_4n_6AmkRxhKSOwC4:1417088721)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"andreimatei"}}