Skip to content

Improving kv test coverage#63

Merged
embark merged 1 commit intocockroachdb:masterfrom
embark:kathy/kv_test_coverage
Sep 17, 2014
Merged

Improving kv test coverage#63
embark merged 1 commit intocockroachdb:masterfrom
embark:kathy/kv_test_coverage

Conversation

@embark
Copy link
Copy Markdown
Contributor

@embark embark commented Sep 16, 2014

  • Adds unit tests to kv/local_kv.go and kv/dist_kv.go
  • Generalizes gossip network simulation so that it can be used in tests which require gossip networks
  • The unit tests for kv/dist_kv found a user permission bug which led to false positives. This is fixed in storage/prefix.go.

I'm brand new to Go, so I would appreciate any advice!

tbg
tbg reviewed Sep 16, 2014
View reviewed changes
gossip/simulation.go Outdated
Copy link
Copy Markdown
Member

@tbg tbg Sep 16, 2014

Choose a reason for hiding this comment

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

Now that we are using this in tests, it would be better to move this line into main() below.

Copy link
Copy Markdown
Contributor Author

@embark embark Sep 17, 2014

Choose a reason for hiding this comment

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

I moved it into the main() of simulation/gossip.go, which I believe is what you intended.

@tbg
Copy link
Copy Markdown
Member

tbg commented Sep 16, 2014

@embark, thanks for contributing! Looks good except for some minor style issues.

spencerkimball
spencerkimball reviewed Sep 16, 2014
View reviewed changes
gossip/simulation.go Outdated
Copy link
Copy Markdown
Member

@spencerkimball spencerkimball Sep 16, 2014

Choose a reason for hiding this comment

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

Might want to reformat this comment to use 80 character line widths.

@spencerkimball
Copy link
Copy Markdown
Member

spencerkimball commented Sep 16, 2014

embark, thanks for the contribution. Certainly it's a very welcome one, providing test coverage for a critical untested part of the code base! I certainly wouldn't have guessed you haven't programmed before in Go.

Do you have any part of the system which you're particularly interested in working on?

@tbg
Copy link
Copy Markdown
Member

tbg commented Sep 17, 2014

LGTM!

spencerkimball
spencerkimball reviewed Sep 17, 2014
View reviewed changes
kv/dist_kv_test.go Outdated
Copy link
Copy Markdown
Member

@spencerkimball spencerkimball Sep 17, 2014

Choose a reason for hiding this comment

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

Wow that's what I call a unittest..

Copy link
Copy Markdown
Contributor Author

@embark embark Sep 17, 2014

Choose a reason for hiding this comment

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

Haha, hopefully not too complicated.

@spencerkimball
Copy link
Copy Markdown
Member

spencerkimball commented Sep 17, 2014

LGTM. Go ahead and submit this. I invited you to the cockroachdb developers team, so you should have write permissions to the repo.

@embark
Adding unit tests to kv/local_kv_test.go
a7a5ffb 
Generalizing gossip network simulation so that it can be used to test gossip networks more broadly

Adding unit tests for kv/dist_kv.go and fix user permission bug in
storage/prefix.go caught by the tests
@embark
Copy link
Copy Markdown
Contributor Author

embark commented Sep 17, 2014

I will try to merge as soon as the build passes -- I have addressed all style comments. Thank you for the suggestions!

Is there any unassigned work to be done with regard to transactions that I could help with?

embark pushed a commit that referenced this pull request Sep 17, 2014
Merge pull request #63 from embark/kathy/kv_test_coverage
c00474e 
Improving kv test coverage
@embark embark merged commit c00474e into cockroachdb:master Sep 17, 2014
@embark embark deleted the kathy/kv_test_coverage branch September 17, 2014 18:30
@spencerkimball
Copy link
Copy Markdown
Member

spencerkimball commented Sep 17, 2014

Well there is an important missing piece of the distributed transactions
puzzle right now: correctly measuring clock offset between nodes,
propagating this info via the gossip network and enforcing it by having
nodes commit suicide as soon as they notice they're out of band.

Further, I'd like to find some way to create a merged histogram between all
nodes for measured clocks skews and surface it in our status endpoint
(probably via gossip to start, though eventually we'll want to write this
information as time series information to the database itself).

The algorithm I have in mind is the one used by ntp:
http://en.wikipedia.org/wiki/Network_Time_Protocol

There are others. This paper discusses their preferred method using linear
programming and compares it to three other algorithms:
https://www.cs.umd.edu/class/spring2007/cmsc711/papers/1999-infocom-clock.pdf

This is a critical component and would be excellent if you could make
progress on it. If you'd rather work on something else, there are other
pieces, but not so self-contained. Let me know.

On Wed, Sep 17, 2014 at 2:25 PM, embark notifications@github.com wrote:

I will try to merge as soon as the build passes -- I have addressed all
style comments. Thank you for the suggestions!

Is there any unassigned work to be done with regard to transactions that I
could help with?

—
Reply to this email directly or view it on GitHub
#63 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@embark @tbg @spencerkimball @cockroach-team