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

Refactor argument handling in db_crashtest.py #3809

Closed
wants to merge 3 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented May 4, 2018

  • Any options unknown to db_crashtest.py are now passed directly to db_stress. This way, we won't need to update db_crashtest.py every time db_stress gets a new option.
  • Remove db_crashtest.py redundant arguments where the value is the same as db_stress's default
  • Remove db_crashtest.py redundant arguments where the value is the same in a previously applied options map. For example, default_params are always applied before whitebox_default_params, so if they require the same value for an argument, that value only needs to be provided in default_params.
  • Made the simple option maps applied in addition to the regular option maps. Previously they were exclusive which led to lots of duplication

Test Plan:

TEST_TMPDIR=/dev/shm/rocksdb OPT=-g make J=1 crash_test

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ajkr ajkr requested review from sagar0 and anand1976 May 8, 2018 17:57
@ajkr ajkr removed their assignment May 8, 2018
@ajkr
Copy link
Contributor Author

ajkr commented May 8, 2018

unassigning myself because I can't review my own PR and (I guess) having it assigned will make others less likely to look.

Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

None yet

3 participants