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.

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

Improve test coverage for releaseRange rollbacks. #37

Merged
merged 1 commit into from
Sep 11, 2017

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Sep 7, 2017

This is aimed at increasing coverage of freelist.releaseRange from the tx level, with a focus on exercising the rollback logic for cases where long running read transactions are holding pages in the pending state.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@3a49aac). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #37   +/-   ##
=========================================
  Coverage          ?   85.14%           
=========================================
  Files             ?        9           
  Lines             ?     1851           
  Branches          ?        0           
=========================================
  Hits              ?     1576           
  Misses            ?      163           
  Partials          ?      112

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a49aac...d065aa7. Read the comment docs.

@jpbetz
Copy link
Contributor Author

jpbetz commented Sep 8, 2017

cc @heyitsanthony @cheftako @mml

tx_test.go Outdated
@@ -602,6 +602,108 @@ func TestTx_CopyFile_Error_Normal(t *testing.T) {
}
}

// Ensure that a update that releases pages in ranges between read transactions
Copy link
Contributor

Choose a reason for hiding this comment

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

To follow go-style function comment, do TestTx_releaseRange ensures that...

Copy link

Choose a reason for hiding this comment

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

Also I think the test comment should say what we mean by "properly". In particular, this is a correctness test that verifies we don't "overfree". (Unless I read the code wrong.)

}
}

put("k1", "v1")
Copy link
Contributor

Choose a reason for hiding this comment

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

the test sequence is deterministic. we could randomize the test sequence to increase our confidence. but i think it is OK for now. better than nothing.

Copy link

Choose a reason for hiding this comment

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

Yeah, definitely better than nothing. Could leave behind a TODO comment about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions. Yes, the goal of this test is fairly modest-- Guarantee that we have coverage of a releaseRange being applied to the txid ranges between long running transactions and that we check that the read transactions can read values from the txids they are pinned to after the release.

I'll flesh out the documentation of this test to make this more clear.

I'm working on a separate quick (randomized) test that might be better used for soak testing. I need to compare it with the existing simulation_test and then will send out an issue summarizing how it would work for review, and if there is interest in a PR, I'll submit one.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jpbetz thanks! make sense to me.

@xiang90
Copy link
Contributor

xiang90 commented Sep 8, 2017

lgtm. defer to @heyitsanthony

Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

lgtm thanks

@xiang90 xiang90 merged commit 4d3ab93 into etcd-io:master Sep 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants