batcheval,storage: move EndTransaction to batcheval #26917

benesch · 2018-06-22T02:19:25Z

Move the last remaining command, EndTransaction, to the batcheval
package, where it logically belongs. In doing so, resolve a longstanding
TODO.

The one-phase commit logic still lives in storage and, unfortunately,
ends up tightly coupled with the EndTransaction machinery in batcheval,
but this is a step in the right direction. By contrast, the extraction
of writeInitial[Replica]State into the stateloader package goes quite
smoothly.

This patch is strictly code movement. Everything was copy/pasted verbatim with the exception of adding/removing package qualifiers as necessary.

Release note: None

cockroach-teamcity · 2018-06-22T02:19:30Z

This change is

nvanbenschoten · 2018-06-22T14:07:40Z

🎊 🎊 🎊

Reviewed 15 of 15 files at r1.
Review status: complete! 0 of 0 LGTMs obtained

pkg/storage/replica_state.go, line 30 at r1 (raw file):

)

// ClusterSettings returns the node's ClusterSettings.

This is a strange file. I wonder if it needs to exist at all anymore.

pkg/storage/storagebase/base.go, line 142 at r1 (raw file):

// belonging to or outside of the descriptor's key range, and passing an intent
// which begins range-local but ends non-local results in a panic.
// TODO(tschottdorf): move to proto, make more gen-purpose - kv.truncate does

You'll probably run into issues making this (or the two above) methods in roachpb because of the dependency on keys, but arguably it could at least live in keys instead of here.

pkg/storage/storagebase/errors.go, line 21 at r1 (raw file):

// NewReplicaCorruptionError creates a new error indicating a corrupt replica,
// with the supplied list of errors given as history.
func NewReplicaCorruptionError(err error) *roachpb.ReplicaCorruptionError {

I think this should actually go in roachpb/errors.go

Comments from Reviewable

benesch · 2018-06-22T17:15:39Z

Good ideas! Now that I'm not hurtling through the air at 600mph in a little tin can I'll take another stab at decomposing this.

benesch · 2018-06-22T17:54:23Z

Review status: complete! 0 of 0 LGTMs obtained

pkg/storage/replica_state.go, line 30 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is a strange file. I wonder if it needs to exist at all anymore.

Yeah, super weird. I smushed into what seems like a logical place in replica.go.

pkg/storage/storagebase/base.go, line 142 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

You'll probably run into issues making this (or the two above) methods in roachpb because of the dependency on keys, but arguably it could at least live in keys instead of here.

Yeah, agreed it doesn't belong here. It looks like nothing else in keys touches roachpb.RangeDescriptors, though, so if it's alright with you I'd prefer to leave it here for now and do a more targeted refactor later. It seems like there's some general grossness with how the ContainsKey method is different from roachpb.RangeDescriptor.ContainsKey and it could all use some cleanup.

pkg/storage/storagebase/errors.go, line 21 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think this should actually go in roachpb/errors.go

Wow, yeah. There's a very obvious place for it. Thanks for the pointer.

Comments from Reviewable

benesch · 2018-06-22T17:54:26Z

Review status: complete! 0 of 0 LGTMs obtained

pkg/storage/replica_state.go, line 30 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is a strange file. I wonder if it needs to exist at all anymore.

Yeah, super weird. I smushed into what seems like a logical place in replica.go.

pkg/storage/storagebase/base.go, line 142 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

You'll probably run into issues making this (or the two above) methods in roachpb because of the dependency on keys, but arguably it could at least live in keys instead of here.

Yeah, agreed it doesn't belong here. It looks like nothing else in keys touches roachpb.RangeDescriptors, though, so if it's alright with you I'd prefer to leave it here for now and do a more targeted refactor later. It seems like there's some general grossness with how the ContainsKey method is different from roachpb.RangeDescriptor.ContainsKey and it could all use some cleanup.

pkg/storage/storagebase/errors.go, line 21 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think this should actually go in roachpb/errors.go

Wow, yeah. There's a very obvious place for it. Thanks for the pointer.

Comments from Reviewable

nvanbenschoten · 2018-06-22T21:51:09Z

Reviewed 9 of 9 files at r2.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/roachpb/errors.go, line 574 at r2 (raw file):

// NewReplicaCorruptionError creates a new error indicating a corrupt replica,
// with the supplied list of errors given as history.

list?

pkg/storage/storagebase/base.go, line 142 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yeah, agreed it doesn't belong here. It looks like nothing else in keys touches roachpb.RangeDescriptors, though, so if it's alright with you I'd prefer to leave it here for now and do a more targeted refactor later. It seems like there's some general grossness with how the ContainsKey method is different from roachpb.RangeDescriptor.ContainsKey and it could all use some cleanup.

SGTM

Comments from Reviewable

benesch · 2018-06-22T22:57:58Z

Review status: complete! 1 of 0 LGTMs obtained

pkg/roachpb/errors.go, line 574 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

list?

Heh, looks like NewReplicaCorruptionError predates errors.Wrap. When it was updated for an errors.Wrap world the comment wasn't updated: 98db634.

I've updated it now.

pkg/storage/storagebase/base.go, line 142 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

SGTM

Ack. (Reviewable seems to think there's more to be done on this comment stream.)

Comments from Reviewable

tbg · 2018-06-23T11:59:01Z

Could you hold this for a minute? I have a relatively urgent mitigation for #26830 that needs to be backported, and it'll make my life a lot easier if I don't have to rewrite the patch on 2.0.

tbg · 2018-06-23T11:59:32Z

(but generally thanks for cleaning this up! This has bothered me a few times myself)

benesch · 2018-06-23T16:44:22Z

Most certainly!

…

On Sat, Jun 23, 2018 at 4:59 AM Tobias Schottdorf ***@***.***> wrote: (but generally thanks for cleaning this up! This has bothered me a few times myself) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#26917 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA15IK7BKf_hMCJmvcH8oAYEtaayntPNks5t_i21gaJpZM4UzCqo> .

tbg · 2018-06-25T11:53:39Z

Fire away! Sorry for the wait. On Sat, Jun 23, 2018 at 12:44 PM Nikhil Benesch <[email protected]> wrote:

Most certainly! On Sat, Jun 23, 2018 at 4:59 AM Tobias Schottdorf < ***@***.***> wrote: > (but generally thanks for cleaning this up! This has bothered me a few > times myself) > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < #26917 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AA15IK7BKf_hMCJmvcH8oAYEtaayntPNks5t_i21gaJpZM4UzCqo > > . > — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#26917 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE135OtNLZIfjFarTXu3BuHyHS5suLPVks5t_nBxgaJpZM4UzCqo> .

--

…

-- Tobias

bdarnell · 2018-06-26T04:10:13Z

Reviewed 8 of 15 files at r1, 9 of 9 files at r2.
Review status: complete! 1 of 0 LGTMs obtained (and 1 stale)

Comments from Reviewable

Move the last remaining command, EndTransaction, to the batcheval package, where it logically belongs. In doing so, resolve a longstanding TODO. The one-phase commit logic still lives in storage and, unfortunately, ends up tightly coupled with the EndTransaction machinery in batcheval, but this is a step in the right direction. By contrast, the extraction of writeInitial[Replica]State into the stateloader package goes quite smoothly. Release note: None

benesch · 2018-06-29T01:58:17Z

TFTRs!

bors r=bdarnell,tschottdorf,nvanbenschoten

craig · 2018-06-29T03:23:30Z

Timed out (retrying...)

benesch · 2018-06-29T03:27:57Z

bors r=bdarnell,tschottdorf,nvanbenschoten

craig · 2018-06-29T03:27:57Z

Not awaiting review

craig · 2018-06-29T04:13:51Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2018-06-29T05:29:50Z

Build failed (retrying...)

GitHub CI (Cockroach)

26917: batcheval,storage: move EndTransaction to batcheval r=bdarnell,tschottdorf,nvanbenschoten a=benesch Move the last remaining command, EndTransaction, to the batcheval package, where it logically belongs. In doing so, resolve a longstanding TODO. The one-phase commit logic still lives in storage and, unfortunately, ends up tightly coupled with the EndTransaction machinery in batcheval, but this is a step in the right direction. By contrast, the extraction of writeInitial[Replica]State into the stateloader package goes quite smoothly. This patch is strictly code movement. Everything was copy/pasted verbatim with the exception of adding/removing package qualifiers as necessary. Release note: None Co-authored-by: Nikhil Benesch <[email protected]>

craig · 2018-06-29T07:00:06Z

Build succeeded

GitHub CI (Cockroach)

benesch requested review from tbg and a team June 22, 2018 02:19

benesch force-pushed the batcheval-et branch from b37bb2d to 018b580 Compare June 22, 2018 17:51

benesch force-pushed the batcheval-et branch from 018b580 to b8982e8 Compare June 29, 2018 01:24

craig bot merged commit b8982e8 into cockroachdb:master Jun 29, 2018

benesch deleted the batcheval-et branch July 3, 2018 15:38

nvanbenschoten mentioned this pull request Sep 25, 2018

storage: move evaluation of BatchRequests to a separate package #18784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batcheval,storage: move EndTransaction to batcheval #26917

batcheval,storage: move EndTransaction to batcheval #26917

benesch commented Jun 22, 2018

cockroach-teamcity commented Jun 22, 2018

nvanbenschoten commented Jun 22, 2018

benesch commented Jun 22, 2018

benesch commented Jun 22, 2018

benesch commented Jun 22, 2018

nvanbenschoten commented Jun 22, 2018

benesch commented Jun 22, 2018

tbg commented Jun 23, 2018

tbg commented Jun 23, 2018

benesch commented Jun 23, 2018 via email

tbg commented Jun 25, 2018 via email

bdarnell commented Jun 26, 2018

benesch commented Jun 29, 2018

craig bot commented Jun 29, 2018

benesch commented Jun 29, 2018

craig bot commented Jun 29, 2018

craig bot commented Jun 29, 2018

craig bot commented Jun 29, 2018

craig bot commented Jun 29, 2018

batcheval,storage: move EndTransaction to batcheval #26917

batcheval,storage: move EndTransaction to batcheval #26917

Conversation

benesch commented Jun 22, 2018

cockroach-teamcity commented Jun 22, 2018

nvanbenschoten commented Jun 22, 2018

benesch commented Jun 22, 2018

benesch commented Jun 22, 2018

benesch commented Jun 22, 2018

nvanbenschoten commented Jun 22, 2018

benesch commented Jun 22, 2018

tbg commented Jun 23, 2018

tbg commented Jun 23, 2018

benesch commented Jun 23, 2018 via email

tbg commented Jun 25, 2018 via email

bdarnell commented Jun 26, 2018

benesch commented Jun 29, 2018

craig bot commented Jun 29, 2018

Timed out (retrying...)

benesch commented Jun 29, 2018

craig bot commented Jun 29, 2018

craig bot commented Jun 29, 2018

Build failed (retrying...)

craig bot commented Jun 29, 2018

Build failed (retrying...)

craig bot commented Jun 29, 2018

Build succeeded