{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/1382","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/1382/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/1382/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/1382/events","html_url":"https://github.com/cockroachdb/cockroach/pull/1382","id":88741058,"number":1382,"title":"notLeaderError should use originRaftNodeID as input parameter.","user":{"login":"es-chow","id":11871102,"avatar_url":"https://avatars.githubusercontent.com/u/11871102?v=3","gravatar_id":"","url":"https://api.github.com/users/es-chow","html_url":"https://github.com/es-chow","followers_url":"https://api.github.com/users/es-chow/followers","following_url":"https://api.github.com/users/es-chow/following{/other_user}","gists_url":"https://api.github.com/users/es-chow/gists{/gist_id}","starred_url":"https://api.github.com/users/es-chow/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/es-chow/subscriptions","organizations_url":"https://api.github.com/users/es-chow/orgs","repos_url":"https://api.github.com/users/es-chow/repos","events_url":"https://api.github.com/users/es-chow/events{/privacy}","received_events_url":"https://api.github.com/users/es-chow/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/cockroachdb/cockroach/labels/PTAL","name":"PTAL","color":"ededed"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":10,"created_at":"2015-06-16T14:17:30Z","updated_at":"2015-06-17T15:52:40Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/1382","html_url":"https://github.com/cockroachdb/cockroach/pull/1382","diff_url":"https://github.com/cockroachdb/cockroach/pull/1382.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/1382.patch"},"body":"Found some odd log \"replica 2:2 not leader; leader is 2:2\" when running following command:\r\n```shell\r\n./cockroach range split a\r\n./cockroach range split b\r\n```\r\nlog snippet:\r\n```log\r\nI0616 21:18:01.923417    9995 range_command.go:754] range 2: new leader lease replica 2:2 21:18:00.000 +2.008s\r\nE0616 21:18:01.923749    9995 range.go:782] error executing raft command InternalResolveIntent: replica 2:2 not leader; leader is 2:2\r\nE0616 21:18:01.924795    9995 range.go:782] error executing raft command InternalPushTxn: replica 2:2 not leader; leader is 2:2\r\n```\r\nThis happens when a InternalLeaderLease command from server 2 is proposed to raft before a InternalResolveIntent/InternalPushTxn command from server 1, so when applyRaftCommand(InternalResolveIntent) is executed, Leader has been changed, a NotLeaderError will be issued with following code snippet. newNotLeaderError method will use current store to find origin leader, the result is itself, so the odd log comes.\r\n```go\r\n        // Verify the leader lease is held; Note that we don't require the                                                                                                      \r\n        // leader lease when trying to grant the leader lease!                                                                                                                  \r\n        if _, ok := args.(*proto.InternalLeaderLeaseRequest); !ok {\r\n                if !r.verifyLeaderLease(originNodeID, header.Timestamp) {\r\n                        err := r.newNotLeaderError()\r\n                        reply.Header().SetGoError(err)\r\n                        return err\r\n                }\r\n        }\r\n```"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/112858185","html_url":"https://github.com/cockroachdb/cockroach/pull/1382#issuecomment-112858185","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/1382","id":112858185,"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},"created_at":"2015-06-17T15:52:40Z","updated_at":"2015-06-17T15:52:40Z","body":"@tschottdorf Ah, I forgot about that defer. \r\n\r\n@es-chow: No, it shouldn't call `r.executeCmd` if there is a `NotLeaderError`. By \"the rest of this method\" I meant that a `NotLeaderError` should be handled in the same way as an error returned by `r.executeCmd`, including the applied index (which it turns out we are doing anyway) and response cache.\r\n\r\nI'm not sure whether putting this in the response cache is the right thing to do or not. Most of the time when there is an error we do put it in the response cache (so if the operation is retried, it will have to get a new command ID). But in the case of a `NotLeaderError` there is nothing wrong with the request, so it could be retried on the new leader without going back to the client for a new command id. It seems that the main benefit to caching NotLeaderErrors is to be able to more easily test this change, but that's probably not worth it. \r\n\r\nI wouldn't worry too much about testing this change since it doesn't seem to have a meaningful effect on anything but logging."}}