{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16159","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16159/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16159/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16159/events","html_url":"https://github.com/cockroachdb/cockroach/pull/16159","id":231511426,"number":16159,"title":"RFC: Raft sideloading for SSTable ingestion","user":{"login":"tschottdorf","id":5076964,"avatar_url":"https://avatars2.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},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2017-05-26T02:43:04Z","updated_at":"2017-05-31T17:16:14Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/16159","html_url":"https://github.com/cockroachdb/cockroach/pull/16159","diff_url":"https://github.com/cockroachdb/cockroach/pull/16159.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/16159.patch"},"body":"This is the sister PR to https://github.com/cockroachdb/cockroach/pull/15895/files.\n\nLittle rough around the edges, but wanted to get this out so that folks can\ntake a look before next week if they want to."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/305255733","html_url":"https://github.com/cockroachdb/cockroach/pull/16159#issuecomment-305255733","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/16159","id":305255733,"user":{"login":"bdarnell","id":160562,"avatar_url":"https://avatars2.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},"created_at":"2017-05-31T17:16:14Z","updated_at":"2017-05-31T17:16:14Z","body":"\n\n\n\nReview status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.\n\n---\n\n*[docs/RFCS/raft_proposal_sideloading.md, line 53 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/16159#-Kl5S1gLq4F29dGB5Tr1:-KlUUHyqTsyny4ra-0F_:b-c7iovu) ([raw file](https://github.com/cockroachdb/cockroach/blob/30dd2b2ea132bd230aecf3b62b5845594510e39e/docs/RFCS/raft_proposal_sideloading.md#L53)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary><blockquote>\n\nWhat is the difference between `append()` and `evalTruncateLog`? They both know all the possible indexes to delete, for example for `append()`:\n\n```diff\ndiff --git a/pkg/storage/replica_raftstorage.go b/pkg/storage/replica_raftstorage.go\nindex 5273b5608..34a00b2a9 100644\n--- a/pkg/storage/replica_raftstorage.go\n+++ b/pkg/storage/replica_raftstorage.go\n@@ -457,6 +457,7 @@ func (r *Replica) append(\n                if ent.Index > prevLastIndex {\n                        err = engine.MVCCBlindPut(ctx, batch, &diff, key, hlc.Timestamp{}, value, nil /* txn */)\n                } else {\n+                       // TODO(tschottdorf): may have to unlink store/sideloaded/<ent.Index>.*\n                        err = engine.MVCCPut(ctx, batch, &diff, key, hlc.Timestamp{}, value, nil /* txn */)\n                }\n                if err != nil {\n@@ -467,6 +468,7 @@ func (r *Replica) append(\n        // Delete any previously appended log entries which never committed.\n        lastIndex := entries[len(entries)-1].Index\n        for i := lastIndex + 1; i <= prevLastIndex; i++ {\n+               // TODO(tschottdorf): may have to unlink store/sideloaded/<ent.Index>.*\n                err := engine.MVCCDelete(ctx, batch, &diff, r.raftMu.stateLoader.RaftLogKey(i),\n                        hlc.Timestamp{}, nil /* txn */)\n                if err != nil {\n```\n\nreplicaGC on the other hand nukes the whole thing, so it would just `rmtree store/sideloaded/<range_id>`, no?\n\nWhat's perhaps worrying is the need for globbing in some deletion scenarios, but that would go away with a `store/sideloaded/<range_id>/<log_index>/<hash>` setup (in the sense that the glob would hit only files it would also get to delete).\n\nI'll think through what the result would be if we disambiguated for both term and replicaID instead, but intuitively that seems harder to juggle. The ideal scheme would never have to look up more than one file, but that seems unlikely given the atomicity constraints.\n</blockquote></details>\n\n`append()` knows the indexes to delete, but it may also need to write a new file at the same index, and these operations are performed out of order. It's manageable, but might be a little simpler if the term were in the filename. ReplicaGC deletes everything, but putting the replica ID in the filename would allow us to release certain locks while the filesystem ops commit (without that, they must be done synchronously under lock). \n\nI'd also avoid using too many subdirectories here - the directories need to be created and deleted separately, which seems like an unnecessary expense/hassle as long as we don't expect so many files that the directory listing gets too large. \n\n---\n\n*[docs/RFCS/raft_proposal_sideloading.md, line 99 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/16159#-Kl5mG-Fw04Pk6e_a9nV:-KlUR6Ojc5w7T2i5vXj2:b3f553g) ([raw file](https://github.com/cockroachdb/cockroach/blob/30dd2b2ea132bd230aecf3b62b5845594510e39e/docs/RFCS/raft_proposal_sideloading.md#L99)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary><blockquote>\n\n`append()` doesn't want to decode all incoming Raft commands though, but it would have to reach all the way into `storagebase.RaftProposal` to get there -- must be missing something, because avoiding that is why I'm trying to make these proposals sniffable in the first place.\n</blockquote></details>\n\nAh, I see. I wasn't clear on the fact that you wanted this flag for use in `Replica.append`. We talked about a cluster setting to control whether RESTORE would generate sideloaded commands; this setting could be the mechanism we use for changing the command version. \n\n---\n\n*[docs/RFCS/raft_proposal_sideloading.md, line 151 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/16159#-Kl4sDLQJWQJ8g7TyTrZ:-KlUU-lpyxTR9Zn1dZXt:b-335lyl) ([raw file](https://github.com/cockroachdb/cockroach/blob/30dd2b2ea132bd230aecf3b62b5845594510e39e/docs/RFCS/raft_proposal_sideloading.md#L151)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary><blockquote>\n\n@danhhz, can you elaborate?\n</blockquote></details>\n\nIngestion sometimes writes a global sequence number [here](https://github.com/facebook/rocksdb/blob/07bdcb91fe1fb6fcbed885580dcded8f5eac36f5/db/external_sst_file_ingestion_job.cc#L500-L528). I'm not sure whether it would be possible for us to avoid this. \n\n---\n\n*[docs/RFCS/raft_proposal_sideloading.md, line 117 at r6](https://reviewable.io:443/reviews/cockroachdb/cockroach/16159#-KlUSj4KWADFlnZU6ThZ:-KlUSj4KWADFlnZU6Th_:b-wsyvd0) ([raw file](https://github.com/cockroachdb/cockroach/blob/ead60ba72d3f309d81631d8290937aadc46cc775/docs/RFCS/raft_proposal_sideloading.md#L117)):*\n> ```Markdown\n> sideloading. A node which does not understand this will simply ignore the bit\n> and not sideload the proposal, which is acceptable as sideloading is a\n> performance optimization only.\n> ```\n\nSideloading is not a performance optimization only if it means that the data is stored in a new field ReplicatedEvalResult.SideloadedData. We won't be able to send commands in this format until we know that all nodes will be able to handle it. \n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/16159)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"bdarnell"}}