Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replica Batch ground work #2340

Merged
merged 1 commit into from
Sep 6, 2015
Merged

Replica Batch ground work #2340

merged 1 commit into from
Sep 6, 2015

Conversation

tbg
Copy link
Member

@tbg tbg commented Sep 3, 2015

opening this as a PR so that we can comment and discuss on the diffs. I rebased on top of master and am adding commits, nothing else. Original branch is backed up here.

@tbg
Copy link
Member Author

tbg commented Sep 3, 2015

the changes up to and including my 5th commit fix intent the push batch in resolveWriteIntentError, un-hide response errors and allow for some support for self-overlapping batches (we need to discuss this again). Basically I fixed TestUpdateRangeAdressing (and probably a lot more).

@tbg tbg force-pushed the spencerkimball/replica-batch branch 2 times, most recently from 898ca66 to 7a9722d Compare September 3, 2015 16:53
@tbg
Copy link
Member Author

tbg commented Sep 3, 2015

the storage tests now actually pass (I amended my commits slightly). kv fails because of #2345 which I'm sure @thundercw will have under control shortly. Everything else seems to pass.
It looks like this is almost done, we just need to have clarity about self-overlapping batches.

tamird
tamird reviewed Sep 3, 2015
View reviewed changes
proto/api.proto
@@ -462,11 +462,14 @@ message PushTxnResponse {
}

// A ResolveIntentRequest is arguments to the ResolveIntent()
// method. It is sent by transaction coordinators and after success
// method. It is esnt by transaction coordinators and after success
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@tbg tbg force-pushed the spencerkimball/replica-batch branch from 7a9722d to 5e3c301 Compare September 3, 2015 20:21
@tbg
Copy link
Member Author

tbg commented Sep 3, 2015

cherry-picked @thundercw's open PR #2345 and voila, green build.

@tbg tbg force-pushed the spencerkimball/replica-batch branch 5 times, most recently from 15c9632 to d47b3a3 Compare September 4, 2015 10:51
@tbg
Copy link
Member Author

tbg commented Sep 4, 2015

ok, so once #2345 lands I think this is actually in good shape to be merged. It needs some rounding around the edges (specifically self-overlap) but that's better solved in follow-up work.

So, please review.

@tbg tbg changed the title [DN{Merge,Review}]: WIP replica batch Replica Batch ground work Sep 4, 2015
@tbg tbg assigned tamird and unassigned spencerkimball Sep 4, 2015
@tbg
Copy link
Member Author

tbg commented Sep 4, 2015

@tamird I've assigned you since @spencerkimball is out and you've already taken a look. Feel free to re-assign to someone else, though.

@tbg tbg added this to the Beta milestone Sep 4, 2015
tamird
tamird reviewed Sep 4, 2015
View reviewed changes
keys/keys.go
if body.Less(proto.KeyMax) {
return nil
if proto.KeyMax.Less(body) {
return NewInvalidRangeMetaKeyError("body of meta2 range lookup is > KeyMax", key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/>/>=/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @thundercw (this is cherry-picked from #2345)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, didn't read this right the first time. This is actually comparing for > (note that the old code returned nil if the condition matched)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it comparing for >= since that's the inverse of Less? the old code looks correct with respect to the comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code above: if KeyMax < key { error }, so > KeyMax errors.
old code: if key < KeyMax { no error } else { error } (so everything >=KeyMax errors).

This isn't just refactor, there's an actual change. KeyMax is now valid (at least in this bit of code).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duh, you're right. carry on!

@tbg tbg force-pushed the spencerkimball/replica-batch branch 2 times, most recently from e182955 to dc6cc7b Compare September 4, 2015 16:10
@tbg
Copy link
Member Author

tbg commented Sep 4, 2015

@tamird: thanks for the review. I think I've addressed it all (see last commit for the diff).

@tbg
Copy link
Member Author

tbg commented Sep 4, 2015

still going to kill some pointer receivers, actually.

@tbg tbg force-pushed the spencerkimball/replica-batch branch from dc6cc7b to ea508ac Compare September 4, 2015 16:15
@tbg
Copy link
Member Author

tbg commented Sep 4, 2015

I'm going to wait for #2345 to get ready, update the cherry pick, squash a little bit and then merge.

@tbg tbg force-pushed the spencerkimball/replica-batch branch from ea508ac to 49cb648 Compare September 4, 2015 17:20
bdarnell
bdarnell reviewed Sep 5, 2015
View reviewed changes
proto/api.proto
@@ -472,6 +472,9 @@ message PushTxnResponse {
// commit them.
message ResolveIntentRequest {
optional RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
// The transaction whose intent is being pushed. The transaction must either
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/pushed/resolved/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually just realized that the comment here is wrong. The Txn can of course be PENDING if a txn has been pushed forward in time. Updating.

@bdarnell
Copy link
Contributor

bdarnell commented Sep 5, 2015

LGTM

tamird
tamird reviewed Sep 5, 2015
View reviewed changes
storage/replica.go
}()

// Remove keys from command queue.
r.endCmds(cmdKeys, bArgs, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tschottdorf I think the contentions scope above can be avoided by putting this in a defer. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went ahead and removed the outer scoping. I think that's clear enough and not error prone in this case.

@tbg tbg force-pushed the spencerkimball/replica-batch branch 4 times, most recently from 809374a to 1e53690 Compare September 6, 2015 10:15
@spencerkimball @tbg
storage-side batch processing.
c42426a 
* feedback on original diff
* rebase fixups
* fix data race
* add missing timestamp on BatchRequest header
  this is cleaned up more in #2141, where only the batch gets
  the headers, but with this fix we avoid one obvious source
  of them.
* 5 instead of 50k intents in TestGCQueueIntentResolution
  ain't nobody got time for that (now).
* do not verifyKeys() if it is a batch
  batch semantics can go either way. Cleaned up more in #2141,
  where keys on the batch header are always a key range (and
  subject to removal, see #2155).
* deal with batch self-overlap in command queue
  even basic tests have this (TestUpdateRangeAddressing by the way)
  and however we deal with this, it should be on Store.
* allow self-overlapping batches
  discussion needed, but if this has a good chance
  of leading to passing tests, I think we should
  go down that route first.
* make RaftCmd.Cmd nullable
* panic on empty batch
* feedback
@tbg tbg force-pushed the spencerkimball/replica-batch branch from 1e53690 to c42426a Compare September 6, 2015 10:22
@tbg
Copy link
Member Author

tbg commented Sep 6, 2015

thanks for reviewing @tamird @bdarnell. #2345 just landed and I'm going to merge this on green. I quashed my work into @spencerkimball's so that we don't end up with a broken history (most changes are small anyway and I amended his message).

tbg added a commit that referenced this pull request Sep 6, 2015
@tbg
Merge pull request #2340 from cockroachdb/spencerkimball/replica-batch
df2e6a3 
Replica Batch ground work
@tbg tbg merged commit df2e6a3 into master Sep 6, 2015
@tbg tbg deleted the spencerkimball/replica-batch branch September 6, 2015 13:43
@tbg tbg mentioned this pull request Sep 6, 2015
Closed
@jess-edwards jess-edwards mentioned this pull request Sep 9, 2015
Closed
78 tasks
@tbg tbg mentioned this pull request Sep 19, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tamird tamird

Labels
None yet
Projects
None yet
Milestone
Beta
Development

Successfully merging this pull request may close these issues.
4 participants
@tbg @tamird @bdarnell @spencerkimball