{"action":"created","comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/comments/23549898","id":23549898,"diff_hunk":"@@ -28,84 +27,67 @@ import (\n // policy allows either the union or intersection of maximum # of\n // versions and maximum age.\n type GarbageCollector struct {\n-\tnow      proto.Timestamp // time at start of GC\n-\tpolicyFn func(key proto.Key) *proto.GCPolicy\n+\texpiration proto.Timestamp\n+\tpolicy     proto.GCPolicy\n }\n \n-// NewGarbageCollector allocates and returns a new GC.\n-func NewGarbageCollector(now proto.Timestamp, policyFn func(key proto.Key) *proto.GCPolicy) *GarbageCollector {\n+// NewGarbageCollector allocates and returns a new GC, with expiration\n+// computed based on current time and policy.TTLSeconds.\n+func NewGarbageCollector(now proto.Timestamp, policy proto.GCPolicy) *GarbageCollector {\n+\tttlNanos := int64(policy.TTLSeconds) * 1E9\n \treturn &GarbageCollector{\n-\t\tnow:      now,\n-\t\tpolicyFn: policyFn,\n+\t\texpiration: proto.Timestamp{WallTime: now.WallTime - ttlNanos},\n+\t\tpolicy:     policy,\n \t}\n }\n \n-// MVCCPrefix returns the full key as prefix for non-version MVCC\n-// keys and otherwise just the encoded key portion of version MVCC keys.\n-func (gc *GarbageCollector) MVCCPrefix(key proto.EncodedKey) int {\n-\tremaining, _ := encoding.DecodeBinary(key)\n-\treturn len(key) - len(remaining)\n-}\n-\n // Filter makes decisions about garbage collection based on the\n // garbage collection policy for batches of values for the same key.\n-// The GC policy is determined via the policyFn specified when the\n-// GarbageCollector was created. Returns a slice of deletions, one\n-// per incoming keys. If an index in the returned array is set to\n-// true, then that value will be garbage collected.\n-func (gc *GarbageCollector) Filter(keys []proto.EncodedKey, values [][]byte) []bool {\n-\tif len(keys) == 1 {\n-\t\treturn nil\n-\t}\n-\ttoDelete := make([]bool, len(keys))\n-\t// Decode the first key and make sure it's an MVCC metadata key.\n-\tdKey, ts, isValue := MVCCDecodeKey(keys[0])\n-\tif isValue {\n-\t\tlog.Errorf(\"unexpected MVCC value encountered: %q\", keys[0])\n-\t\treturn toDelete\n+// Returns the timestamp including, and after which, all values should\n+// be garbage collected. If no values should be GC'd, returns\n+// proto.ZeroTimestamp.\n+func (gc *GarbageCollector) Filter(keys []proto.EncodedKey, values [][]byte) proto.Timestamp {\n+\tif gc.policy.TTLSeconds <= 0 {\n+\t\treturn proto.ZeroTimestamp\n \t}\n-\t// Using first key, look up the policy which applies to this set of MVCC values.\n-\tpolicy := gc.policyFn(dKey)\n-\tif policy == nil || policy.TTLSeconds <= 0 {\n-\t\treturn nil\n+\tif len(keys) == 0 {\n+\t\treturn proto.ZeroTimestamp\n \t}\n-\texpiration := gc.now\n-\texpiration.WallTime -= int64(policy.TTLSeconds) * 1E9\n \n-\tvar survivors bool\n \t// Loop over remaining values. All should be MVCC versions.\n-\tfor i, key := range keys[1:] {\n-\t\t_, ts, isValue = MVCCDecodeKey(key)\n+\tvar delTS proto.Timestamp\n+\tvar survivors bool\n+\tfor i, key := range keys {\n+\t\t_, ts, isValue := MVCCDecodeKey(key)\n \t\tif !isValue {\n \t\t\tlog.Errorf(\"unexpected MVCC metadata encountered: %q\", key)\n-\t\t\treturn make([]bool, len(keys))\n+\t\t\treturn proto.ZeroTimestamp\n \t\t}\n \t\tmvccVal := proto.MVCCValue{}\n-\t\tif err := gogoproto.Unmarshal(values[i+1], &mvccVal); err != nil {\n+\t\tif err := gogoproto.Unmarshal(values[i], &mvccVal); err != nil {\n \t\t\tlog.Errorf(\"unable to unmarshal MVCC value %q: %v\", key, err)\n-\t\t\treturn make([]bool, len(keys))\n+\t\t\treturn proto.ZeroTimestamp\n \t\t}\n \t\tif i == 0 {\n-\t\t\t// If the first value isn't a deletion tombstone, set survivors to true.\n+\t\t\t// If the first value isn't a deletion tombstone, don't con\n \t\t\tif !mvccVal.Deleted {\n \t\t\t\tsurvivors = true\n-\t\t\t}\n-\t\t} else {\n-\t\t\tif ts.Less(expiration) {\n-\t\t\t\t// If we encounter a version older than our GC timestamp, mark for deletion.\n-\t\t\t\ttoDelete[i+1] = true\n-\t\t\t} else if !mvccVal.Deleted {\n-\t\t\t\t// Otherwise, if not marked for GC and not a tombstone, set survivors true.\n-\t\t\t\tsurvivors = true\n+\t\t\t\tcontinue\n \t\t\t}\n \t\t}\n+\t\t// If we encounter a version older than our GC timestamp, mark for deletion.\n+\t\tif ts.Less(gc.expiration) {\n+\t\t\tdelTS = ts\n+\t\t\tbreak\n+\t\t} else if !mvccVal.Deleted {\n+\t\t\tsurvivors = true\n+\t\t}\n \t}\n-\t// If there are no remaining non-deleted, versioned entries, mark\n-\t// all keys for deletion, including the MVCC metadata entry.\n+\t// If there are no non-deleted survivors, return timestamp of first key","path":"storage/engine/gc.go","position":118,"original_position":118,"commit_id":"1abe5a227d8c5ba031b60112052ba4875a3f385d","original_commit_id":"1abe5a227d8c5ba031b60112052ba4875a3f385d","user":{"login":"bdarnell","id":160562,"avatar_url":"https://avatars.githubusercontent.com/u/160562?v=3","gravatar_id":"","url":"https://api.github.com/users/bdarnell","html_url":"https://github.com/bdarnell","followers_url":"https://api.github.com/users/bdarnell/followers","following_url":"https://api.github.com/users/bdarnell/following{/other_user}","gists_url":"https://api.github.com/users/bdarnell/gists{/gist_id}","starred_url":"https://api.github.com/users/bdarnell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bdarnell/subscriptions","organizations_url":"https://api.github.com/users/bdarnell/orgs","repos_url":"https://api.github.com/users/bdarnell/repos","events_url":"https://api.github.com/users/bdarnell/events{/privacy}","received_events_url":"https://api.github.com/users/bdarnell/received_events","type":"User","site_admin":false},"body":"This special case strikes me as a premature optimization. Why not just let tombstones be deleted according to the usual rules? ","created_at":"2015-01-26T17:58:54Z","updated_at":"2015-01-26T17:58:54Z","html_url":"https://github.com/cockroachdb/cockroach/pull/288#discussion_r23549898","pull_request_url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/288","_links":{"self":{"href":"https://api.github.com/repos/cockroachdb/cockroach/pulls/comments/23549898"},"html":{"href":"https://github.com/cockroachdb/cockroach/pull/288#discussion_r23549898"},"pull_request":{"href":"https://api.github.com/repos/cockroachdb/cockroach/pulls/288"}}},"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/288","id":27984880,"html_url":"https://github.com/cockroachdb/cockroach/pull/288","diff_url":"https://github.com/cockroachdb/cockroach/pull/288.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/288.patch","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/288","number":288,"state":"open","locked":false,"title":"Spencerkimball/scan queue process","user":{"login":"spencerkimball","id":6147149,"avatar_url":"https://avatars.githubusercontent.com/u/6147149?v=3","gravatar_id":"","url":"https://api.github.com/users/spencerkimball","html_url":"https://github.com/spencerkimball","followers_url":"https://api.github.com/users/spencerkimball/followers","following_url":"https://api.github.com/users/spencerkimball/following{/other_user}","gists_url":"https://api.github.com/users/spencerkimball/gists{/gist_id}","starred_url":"https://api.github.com/users/spencerkimball/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/spencerkimball/subscriptions","organizations_url":"https://api.github.com/users/spencerkimball/orgs","repos_url":"https://api.github.com/users/spencerkimball/repos","events_url":"https://api.github.com/users/spencerkimball/events{/privacy}","received_events_url":"https://api.github.com/users/spencerkimball/received_events","type":"User","site_admin":false},"body":"Implementation of scanQueue.process().\r\n\r\nReworked engine.GarbageCollector's API to simplify it and return only the timestamp of the first value to GC.\r\nAdded InternalGC command and MVCCGarbageCollect. Integration of new GC and call through to range's\r\nInternalGC method in scanQueue.process().","created_at":"2015-01-25T00:17:19Z","updated_at":"2015-01-26T17:58:54Z","closed_at":null,"merged_at":null,"merge_commit_sha":"9002110e993de55efced4ae1ea6f318161c74b43","assignee":{"login":"bdarnell","id":160562,"avatar_url":"https://avatars.githubusercontent.com/u/160562?v=3","gravatar_id":"","url":"https://api.github.com/users/bdarnell","html_url":"https://github.com/bdarnell","followers_url":"https://api.github.com/users/bdarnell/followers","following_url":"https://api.github.com/users/bdarnell/following{/other_user}","gists_url":"https://api.github.com/users/bdarnell/gists{/gist_id}","starred_url":"https://api.github.com/users/bdarnell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bdarnell/subscriptions","organizations_url":"https://api.github.com/users/bdarnell/orgs","repos_url":"https://api.github.com/users/bdarnell/repos","events_url":"https://api.github.com/users/bdarnell/events{/privacy}","received_events_url":"https://api.github.com/users/bdarnell/received_events","type":"User","site_admin":false},"milestone":null,"commits_url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/288/commits","review_comments_url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/288/comments","review_comment_url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/comments/{number}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/288/comments","statuses_url":"https://api.github.com/repos/cockroachdb/cockroach/statuses/1abe5a227d8c5ba031b60112052ba4875a3f385d","head":{"label":"cockroachdb:spencerkimball/scan-queue-process","ref":"spencerkimball/scan-queue-process","sha":"1abe5a227d8c5ba031b60112052ba4875a3f385d","user":{"login":"cockroachdb","id":6748139,"avatar_url":"https://avatars.githubusercontent.com/u/6748139?v=3","gravatar_id":"","url":"https://api.github.com/users/cockroachdb","html_url":"https://github.com/cockroachdb","followers_url":"https://api.github.com/users/cockroachdb/followers","following_url":"https://api.github.com/users/cockroachdb/following{/other_user}","gists_url":"https://api.github.com/users/cockroachdb/gists{/gist_id}","starred_url":"https://api.github.com/users/cockroachdb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cockroachdb/subscriptions","organizations_url":"https://api.github.com/users/cockroachdb/orgs","repos_url":"https://api.github.com/users/cockroachdb/repos","events_url":"https://api.github.com/users/cockroachdb/events{/privacy}","received_events_url":"https://api.github.com/users/cockroachdb/received_events","type":"Organization","site_admin":false},"repo":{"id":16563587,"name":"cockroach","full_name":"cockroachdb/cockroach","owner":{"login":"cockroachdb","id":6748139,"avatar_url":"https://avatars.githubusercontent.com/u/6748139?v=3","gravatar_id":"","url":"https://api.github.com/users/cockroachdb","html_url":"https://github.com/cockroachdb","followers_url":"https://api.github.com/users/cockroachdb/followers","following_url":"https://api.github.com/users/cockroachdb/following{/other_user}","gists_url":"https://api.github.com/users/cockroachdb/gists{/gist_id}","starred_url":"https://api.github.com/users/cockroachdb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cockroachdb/subscriptions","organizations_url":"https://api.github.com/users/cockroachdb/orgs","repos_url":"https://api.github.com/users/cockroachdb/repos","events_url":"https://api.github.com/users/cockroachdb/events{/privacy}","received_events_url":"https://api.github.com/users/cockroachdb/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/cockroachdb/cockroach","description":"A Scalable, Geo-Replicated, Transactional Datastore","fork":false,"url":"https://api.github.com/repos/cockroachdb/cockroach","forks_url":"https://api.github.com/repos/cockroachdb/cockroach/forks","keys_url":"https://api.github.com/repos/cockroachdb/cockroach/keys{/key_id}","collaborators_url":"https://api.github.com/repos/cockroachdb/cockroach/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/cockroachdb/cockroach/teams","hooks_url":"https://api.github.com/repos/cockroachdb/cockroach/hooks","issue_events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/events{/number}","events_url":"https://api.github.com/repos/cockroachdb/cockroach/events","assignees_url":"https://api.github.com/repos/cockroachdb/cockroach/assignees{/user}","branches_url":"https://api.github.com/repos/cockroachdb/cockroach/branches{/branch}","tags_url":"https://api.github.com/repos/cockroachdb/cockroach/tags","blobs_url":"https://api.github.com/repos/cockroachdb/cockroach/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/cockroachdb/cockroach/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/cockroachdb/cockroach/git/refs{/sha}","trees_url":"https://api.github.com/repos/cockroachdb/cockroach/git/trees{/sha}","statuses_url":"https://api.github.com/repos/cockroachdb/cockroach/statuses/{sha}","languages_url":"https://api.github.com/repos/cockroachdb/cockroach/languages","stargazers_url":"https://api.github.com/repos/cockroachdb/cockroach/stargazers","contributors_url":"https://api.github.com/repos/cockroachdb/cockroach/contributors","subscribers_url":"https://api.github.com/repos/cockroachdb/cockroach/subscribers","subscription_url":"https://api.github.com/repos/cockroachdb/cockroach/subscription","commits_url":"https://api.github.com/repos/cockroachdb/cockroach/commits{/sha}","git_commits_url":"https://api.github.com/repos/cockroachdb/cockroach/git/commits{/sha}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/comments{/number}","issue_comment_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/{number}","contents_url":"https://api.github.com/repos/cockroachdb/cockroach/contents/{+path}","compare_url":"https://api.github.com/repos/cockroachdb/cockroach/compare/{base}...{head}","merges_url":"https://api.github.com/repos/cockroachdb/cockroach/merges","archive_url":"https://api.github.com/repos/cockroachdb/cockroach/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/cockroachdb/cockroach/downloads","issues_url":"https://api.github.com/repos/cockroachdb/cockroach/issues{/number}","pulls_url":"https://api.github.com/repos/cockroachdb/cockroach/pulls{/number}","milestones_url":"https://api.github.com/repos/cockroachdb/cockroach/milestones{/number}","notifications_url":"https://api.github.com/repos/cockroachdb/cockroach/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/labels{/name}","releases_url":"https://api.github.com/repos/cockroachdb/cockroach/releases{/id}","created_at":"2014-02-06T00:18:47Z","updated_at":"2015-01-26T17:26:34Z","pushed_at":"2015-01-26T17:26:36Z","git_url":"git://github.com/cockroachdb/cockroach.git","ssh_url":"git@github.com:cockroachdb/cockroach.git","clone_url":"https://github.com/cockroachdb/cockroach.git","svn_url":"https://github.com/cockroachdb/cockroach","homepage":null,"size":13539,"stargazers_count":2670,"watchers_count":2670,"language":"Go","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":184,"mirror_url":null,"open_issues_count":26,"forks":184,"open_issues":26,"watchers":2670,"default_branch":"master"}},"base":{"label":"cockroachdb:master","ref":"master","sha":"c0e49e2206a812a41bd0881b4f43403b2b12e36e","user":{"login":"cockroachdb","id":6748139,"avatar_url":"https://avatars.githubusercontent.com/u/6748139?v=3","gravatar_id":"","url":"https://api.github.com/users/cockroachdb","html_url":"https://github.com/cockroachdb","followers_url":"https://api.github.com/users/cockroachdb/followers","following_url":"https://api.github.com/users/cockroachdb/following{/other_user}","gists_url":"https://api.github.com/users/cockroachdb/gists{/gist_id}","starred_url":"https://api.github.com/users/cockroachdb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cockroachdb/subscriptions","organizations_url":"https://api.github.com/users/cockroachdb/orgs","repos_url":"https://api.github.com/users/cockroachdb/repos","events_url":"https://api.github.com/users/cockroachdb/events{/privacy}","received_events_url":"https://api.github.com/users/cockroachdb/received_events","type":"Organization","site_admin":false},"repo":{"id":16563587,"name":"cockroach","full_name":"cockroachdb/cockroach","owner":{"login":"cockroachdb","id":6748139,"avatar_url":"https://avatars.githubusercontent.com/u/6748139?v=3","gravatar_id":"","url":"https://api.github.com/users/cockroachdb","html_url":"https://github.com/cockroachdb","followers_url":"https://api.github.com/users/cockroachdb/followers","following_url":"https://api.github.com/users/cockroachdb/following{/other_user}","gists_url":"https://api.github.com/users/cockroachdb/gists{/gist_id}","starred_url":"https://api.github.com/users/cockroachdb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cockroachdb/subscriptions","organizations_url":"https://api.github.com/users/cockroachdb/orgs","repos_url":"https://api.github.com/users/cockroachdb/repos","events_url":"https://api.github.com/users/cockroachdb/events{/privacy}","received_events_url":"https://api.github.com/users/cockroachdb/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/cockroachdb/cockroach","description":"A Scalable, Geo-Replicated, Transactional Datastore","fork":false,"url":"https://api.github.com/repos/cockroachdb/cockroach","forks_url":"https://api.github.com/repos/cockroachdb/cockroach/forks","keys_url":"https://api.github.com/repos/cockroachdb/cockroach/keys{/key_id}","collaborators_url":"https://api.github.com/repos/cockroachdb/cockroach/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/cockroachdb/cockroach/teams","hooks_url":"https://api.github.com/repos/cockroachdb/cockroach/hooks","issue_events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/events{/number}","events_url":"https://api.github.com/repos/cockroachdb/cockroach/events","assignees_url":"https://api.github.com/repos/cockroachdb/cockroach/assignees{/user}","branches_url":"https://api.github.com/repos/cockroachdb/cockroach/branches{/branch}","tags_url":"https://api.github.com/repos/cockroachdb/cockroach/tags","blobs_url":"https://api.github.com/repos/cockroachdb/cockroach/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/cockroachdb/cockroach/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/cockroachdb/cockroach/git/refs{/sha}","trees_url":"https://api.github.com/repos/cockroachdb/cockroach/git/trees{/sha}","statuses_url":"https://api.github.com/repos/cockroachdb/cockroach/statuses/{sha}","languages_url":"https://api.github.com/repos/cockroachdb/cockroach/languages","stargazers_url":"https://api.github.com/repos/cockroachdb/cockroach/stargazers","contributors_url":"https://api.github.com/repos/cockroachdb/cockroach/contributors","subscribers_url":"https://api.github.com/repos/cockroachdb/cockroach/subscribers","subscription_url":"https://api.github.com/repos/cockroachdb/cockroach/subscription","commits_url":"https://api.github.com/repos/cockroachdb/cockroach/commits{/sha}","git_commits_url":"https://api.github.com/repos/cockroachdb/cockroach/git/commits{/sha}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/comments{/number}","issue_comment_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/{number}","contents_url":"https://api.github.com/repos/cockroachdb/cockroach/contents/{+path}","compare_url":"https://api.github.com/repos/cockroachdb/cockroach/compare/{base}...{head}","merges_url":"https://api.github.com/repos/cockroachdb/cockroach/merges","archive_url":"https://api.github.com/repos/cockroachdb/cockroach/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/cockroachdb/cockroach/downloads","issues_url":"https://api.github.com/repos/cockroachdb/cockroach/issues{/number}","pulls_url":"https://api.github.com/repos/cockroachdb/cockroach/pulls{/number}","milestones_url":"https://api.github.com/repos/cockroachdb/cockroach/milestones{/number}","notifications_url":"https://api.github.com/repos/cockroachdb/cockroach/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/labels{/name}","releases_url":"https://api.github.com/repos/cockroachdb/cockroach/releases{/id}","created_at":"2014-02-06T00:18:47Z","updated_at":"2015-01-26T17:26:34Z","pushed_at":"2015-01-26T17:26:36Z","git_url":"git://github.com/cockroachdb/cockroach.git","ssh_url":"git@github.com:cockroachdb/cockroach.git","clone_url":"https://github.com/cockroachdb/cockroach.git","svn_url":"https://github.com/cockroachdb/cockroach","homepage":null,"size":13539,"stargazers_count":2670,"watchers_count":2670,"language":"Go","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":184,"mirror_url":null,"open_issues_count":26,"forks":184,"open_issues":26,"watchers":2670,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/cockroachdb/cockroach/pulls/288"},"html":{"href":"https://github.com/cockroachdb/cockroach/pull/288"},"issue":{"href":"https://api.github.com/repos/cockroachdb/cockroach/issues/288"},"comments":{"href":"https://api.github.com/repos/cockroachdb/cockroach/issues/288/comments"},"review_comments":{"href":"https://api.github.com/repos/cockroachdb/cockroach/pulls/288/comments"},"review_comment":{"href":"https://api.github.com/repos/cockroachdb/cockroach/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/cockroachdb/cockroach/pulls/288/commits"},"statuses":{"href":"https://api.github.com/repos/cockroachdb/cockroach/statuses/1abe5a227d8c5ba031b60112052ba4875a3f385d"}}}}