{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16751","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16751/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16751/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16751/events","html_url":"https://github.com/cockroachdb/cockroach/pull/16751","id":239034203,"number":16751,"title":"sql: split AcquireMinVersion() from table lease Acquire()","user":{"login":"vivekmenezes","id":11774212,"avatar_url":"https://avatars2.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":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-06-28T02:17:53Z","updated_at":"2017-07-03T23:46:53Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/16751","html_url":"https://github.com/cockroachdb/cockroach/pull/16751","diff_url":"https://github.com/cockroachdb/cockroach/pull/16751.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/16751.patch"},"body":"This PR is only for the last commit. The other commits are in another PR."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/312750703","html_url":"https://github.com/cockroachdb/cockroach/pull/16751#issuecomment-312750703","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16751","id":312750703,"user":{"login":"tamird","id":1535036,"avatar_url":"https://avatars3.githubusercontent.com/u/1535036?v=3","gravatar_id":"","url":"https://api.github.com/users/tamird","html_url":"https://github.com/tamird","followers_url":"https://api.github.com/users/tamird/followers","following_url":"https://api.github.com/users/tamird/following{/other_user}","gists_url":"https://api.github.com/users/tamird/gists{/gist_id}","starred_url":"https://api.github.com/users/tamird/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tamird/subscriptions","organizations_url":"https://api.github.com/users/tamird/orgs","repos_url":"https://api.github.com/users/tamird/repos","events_url":"https://api.github.com/users/tamird/events{/privacy}","received_events_url":"https://api.github.com/users/tamird/received_events","type":"User","site_admin":false},"created_at":"2017-07-03T23:46:53Z","updated_at":"2017-07-03T23:46:53Z","body":"This doesn't seem simpler to me, but perhaps I'm missing the bigger picture; AFAICT, this replaces \"pass zero to get latest\" with a new method; I think it'd be just as clear with a constant named `latestTableVersion` with a value of zero.\n\nEither way, I don't feel strongly, so LGTM with nits fixed.\n\n---\n\nReviewed 5 of 5 files at r1.\nReview status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.\n\n---\n\n*[pkg/sql/lease.go, line 546 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16751#-Ko9pYvQ5akcI50l9Qyd:-Ko9pYvQ5akcI50l9Qye:b-75azvz) ([raw file](https://github.com/cockroachdb/cockroach/blob/7c561b449d5ccd1e81c2762fcaf31e16195e07bc/pkg/sql/lease.go#L546)):*\n> ```Go\n> \n> // acquire returns the latest version of a table. The table will have its\n> // refcount incremented, so the caller is responsible to call release() on it.\n> ```\n\nwhile you're here: s/to call/for calling/\n\n---\n\n*[pkg/sql/lease.go, line 547 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16751#-Ko9pG6CI_-B4MswuuD9:-Ko9pG6CI_-B4MswuuDA:buio9t3) ([raw file](https://github.com/cockroachdb/cockroach/blob/7c561b449d5ccd1e81c2762fcaf31e16195e07bc/pkg/sql/lease.go#L547)):*\n> ```Go\n> // acquire returns the latest version of a table. The table will have its\n> // refcount incremented, so the caller is responsible to call release() on it.\n> func (t *tableState) acquire(\n> ```\n\nperhaps `acquireLatest` and `acquireVersion` would be better names for these methods?\n\n---\n\n*[pkg/sql/lease.go, line 555 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16751#-Ko9petRUF53FNcjiQT2:-Ko9petS9zXOzSD5qCLN:b-mxzzmt) ([raw file](https://github.com/cockroachdb/cockroach/blob/7c561b449d5ccd1e81c2762fcaf31e16195e07bc/pkg/sql/lease.go#L555)):*\n> ```Go\n> \tfor {\n> \t\ts := t.active.findNewest()\n> \t\tif s != nil && s.hasSomeLifeLeft(m.clock) {\n> ```\n\nnit: `if s := ...; s != nil && .. {` since `s` isn't used outside this scope.\n\n---\n\n*[pkg/sql/lease.go, line 568 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16751#-Ko9pma00aUHdwZoeVht:-Ko9pma00aUHdwZoeVhu:buc8elo) ([raw file](https://github.com/cockroachdb/cockroach/blob/7c561b449d5ccd1e81c2762fcaf31e16195e07bc/pkg/sql/lease.go#L568)):*\n> ```Go\n> \n> // acquireMinVersion returns the latest version >= minVersion. The table\n> // will have its refcount incremented, so the caller is responsible to\n> ```\n\nditto grammar\n\n---\n\n*[pkg/sql/lease.go, line 577 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16751#-Ko9pokMT8czCZCoe1f7:-Ko9pokMT8czCZCoe1f8:bvxsfan) ([raw file](https://github.com/cockroachdb/cockroach/blob/7c561b449d5ccd1e81c2762fcaf31e16195e07bc/pkg/sql/lease.go#L577)):*\n> ```Go\n> \n> \tfor {\n> \t\ts := t.active.findNewest()\n> ```\n\nditto reduce scope of `s`\n\n---\n\n*[pkg/sql/lease_test.go, line 250 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/16751#-Ko9p9MiVnmSAB1mH_G0:-Ko9p9MjTnKL9c7YS0oE:b1qxqbb) ([raw file](https://github.com/cockroachdb/cockroach/blob/cdfc68af81c962831033098551de3846e409cec1/pkg/sql/lease_test.go#L250)):*\n> ```Go\n> \tt.expectLeases(descID, \"/2/1\")\n> \n> \t// It is an error to acquire a lease for an old version once a new version\n> ```\n\nis this no longer the case?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/16751#-:-Ko9q0Dir-i8xF_7tTNC:b5s2t78)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"tamird"}}