{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6831","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6831/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6831/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6831/events","html_url":"https://github.com/cockroachdb/cockroach/pull/6831","id":156016321,"number":6831,"title":"storage: set the last GC expiration time on replicas","user":{"login":"mjibson","id":41181,"avatar_url":"https://avatars.githubusercontent.com/u/41181?v=3","gravatar_id":"","url":"https://api.github.com/users/mjibson","html_url":"https://github.com/mjibson","followers_url":"https://api.github.com/users/mjibson/followers","following_url":"https://api.github.com/users/mjibson/following{/other_user}","gists_url":"https://api.github.com/users/mjibson/gists{/gist_id}","starred_url":"https://api.github.com/users/mjibson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mjibson/subscriptions","organizations_url":"https://api.github.com/users/mjibson/orgs","repos_url":"https://api.github.com/users/mjibson/repos","events_url":"https://api.github.com/users/mjibson/events{/privacy}","received_events_url":"https://api.github.com/users/mjibson/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"tschottdorf","id":5076964,"avatar_url":"https://avatars.githubusercontent.com/u/5076964?v=3","gravatar_id":"","url":"https://api.github.com/users/tschottdorf","html_url":"https://github.com/tschottdorf","followers_url":"https://api.github.com/users/tschottdorf/followers","following_url":"https://api.github.com/users/tschottdorf/following{/other_user}","gists_url":"https://api.github.com/users/tschottdorf/gists{/gist_id}","starred_url":"https://api.github.com/users/tschottdorf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tschottdorf/subscriptions","organizations_url":"https://api.github.com/users/tschottdorf/orgs","repos_url":"https://api.github.com/users/tschottdorf/repos","events_url":"https://api.github.com/users/tschottdorf/events{/privacy}","received_events_url":"https://api.github.com/users/tschottdorf/received_events","type":"User","site_admin":false},"milestone":null,"comments":4,"created_at":"2016-05-20T17:49:43Z","updated_at":"2016-05-24T14:53:31Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/6831","html_url":"https://github.com/cockroachdb/cockroach/pull/6831","diff_url":"https://github.com/cockroachdb/cockroach/pull/6831.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/6831.patch"},"body":"This is in preparation for time travel queries (#5963). It will work in\r\nconjunction with #6778, but does not require that PR to go in first. A\r\nfuture PR, after these are both merged, will enforce reads and writes\r\nhappen after the last GC expiration of a replica.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/6831)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/221297020","html_url":"https://github.com/cockroachdb/cockroach/pull/6831#issuecomment-221297020","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6831","id":221297020,"user":{"login":"tschottdorf","id":5076964,"avatar_url":"https://avatars.githubusercontent.com/u/5076964?v=3","gravatar_id":"","url":"https://api.github.com/users/tschottdorf","html_url":"https://github.com/tschottdorf","followers_url":"https://api.github.com/users/tschottdorf/followers","following_url":"https://api.github.com/users/tschottdorf/following{/other_user}","gists_url":"https://api.github.com/users/tschottdorf/gists{/gist_id}","starred_url":"https://api.github.com/users/tschottdorf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tschottdorf/subscriptions","organizations_url":"https://api.github.com/users/tschottdorf/orgs","repos_url":"https://api.github.com/users/tschottdorf/repos","events_url":"https://api.github.com/users/tschottdorf/events{/privacy}","received_events_url":"https://api.github.com/users/tschottdorf/received_events","type":"User","site_admin":false},"created_at":"2016-05-24T14:53:30Z","updated_at":"2016-05-24T14:53:30Z","body":"Needs a test. Content-wise LGTM mod the timestamp, most of my comments are about naming and commentary.\n\n<details><summary><i>Previously, mjibson (Matt Jibson) wrote…</i></summary>\n> #### storage: set the last GC expiration time on replicas\n> \n> This is in preparation for time travel queries (#5963). It will work in\r> \n> conjunction with #6778, but does not require that PR to go in first. A\r> \n> future PR, after these are both merged, will enforce reads and writes\r> \n> happen after the last GC expiration of a replica.\n> \n> \n> \n</details>\n\n\n---\n\nReview status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.\n\n---\n\n*[storage/gc_queue.go, line 372 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6831#-KIYDm4Y_C6NeGtHXANc:-KIYDm4Y_C6NeGtHXANd:-1192952494) ([raw file](https://github.com/cockroachdb/cockroach/blob/6725221ad276a959c7d57d72557721caa78478c0/storage/gc_queue.go#L372)):*\n> ```Go\n> \t// ResolveErrors is the number of successful intent resolutions.\n> \tResolveSuccess int\n> \t// Expiration is the computed expiration timestamp in ns.\n> ```\n\nIf I didn't know what this was already, I wouldn't know from the description. Expand the comment (similar to what I suggest in `(Replica).lastGC`) and/or rename this field - `Threshold`?\n\n---\n\n*[storage/replica.go, line 227 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6831#-KIJ73VSyMx8BTpeVN5z:-KIYDGxKcvEjgQTef2ic:713830474) ([raw file](https://github.com/cockroachdb/cockroach/blob/6725221ad276a959c7d57d72557721caa78478c0/storage/replica.go#L227)):*\n<details><summary><i>Previously, bdarnell (Ben Darnell) wrote…</i></summary>\n> It doesn't have a `SetTimestamp`, but it does have a `SetProto` (and `roachpb.Timestamp` implements `proto.Message`). After thinking about this some more I'm not sure whether it really matters here, though. We're always constructing these timestamps based on wall time, never deriving them from the MVCC values. It's probably still better to store the `roachpb.Timestamp`, though - that could be useful if we introduce other GC policies that are not based on fixed time intervals. \n</details>\nYeah, better to make this a `roachpb.Timestamp`. Also, the description is incorrect (unless you're doing something other than what we've discussed). This timestamp should be the threshold used by the last GC, i.e. the timestamp below which reads are disallowed because the data they would read might've been GC'ed.\nIt should also be renamed - maybe to `lastGCThreshold` (assuming you rename `Expiration` to `Threshold` in `GCInfo`), with the rename trickling down to other places (`setLastGC`, etc). Now that I think of it, `gcThreshold` is even better - it's the max of all gc thresholds ever, so nothing specific about the last gc.\n\n---\n\n*[storage/replica_command.go, line 1033 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6831#-KIYE_3-6OLvmLzu7A51:-KIYE_3-6OLvmLzu7A52:1268388891) ([raw file](https://github.com/cockroachdb/cockroach/blob/6725221ad276a959c7d57d72557721caa78478c0/storage/replica_command.go#L1033)):*\n> ```Go\n> \tupdateLast := args.Expiration > r.mu.lastGC\n> \tr.mu.Unlock()\n> \tif !updateLast {\n> ```\n\nI don't think it's worth to guard this - just always run it.\n\n---\n\n*[storage/replica_command.go, line 1039 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6831#-KIYEQJPhPLZJn0CegyF:-KIYEQJPhPLZJn0CegyG:-718661750) ([raw file](https://github.com/cockroachdb/cockroach/blob/6725221ad276a959c7d57d72557721caa78478c0/storage/replica_command.go#L1039)):*\n> ```Go\n> \tbatch.Defer(func() {\n> \t\tr.mu.Lock()\n> \t\tr.mu.lastGC = args.Expiration\n> ```\n\nYou'll be able to simply `r.mu.lastGCThreshold.Forward(args.Expiration)`.\n\n---\n\n*[storage/engine/gc.go, line 29 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6831#-KIYEkJ9vI-9ryVtyJmy:-KIYEkJ9vI-9ryVtyJmz:1567893725) ([raw file](https://github.com/cockroachdb/cockroach/blob/6725221ad276a959c7d57d72557721caa78478c0/storage/engine/gc.go#L29)):*\n> ```Go\n> // versions and maximum age.\n> type GarbageCollector struct {\n> \tExpiration roachpb.Timestamp\n> ```\n\nAh, that's where `Expiration` comes from. If you think that is a clear name for someone who doesn't know what's going on, keep it and just add comments in the other places (it's reasonably clear here because of context, but I think less so when the name is used elsewhere), though I personally would find a better name.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/6831#-:-KIYFJdDWa-ajO5YkO2c:-278489067)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"tschottdorf"}}