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

compaction_style and compaction_pri should output their value as a st… #1817

Closed
wants to merge 1 commit into from
Closed

compaction_style and compaction_pri should output their value as a st… #1817

wants to merge 1 commit into from

Conversation

highker
Copy link

@highker highker commented Jan 30, 2017

…ring

Summary:
Replace the numerical output for compaction_style and compaction_pri
with strings

Test Plan:
Manually tested:
before:
2017/01/25-13:17:42.948086 7f09a3da59c0 Options.compaction_style: 0
2017/01/25-13:17:42.948087 7f09a3da59c0 Options.compaction_pri: 0
after:
2017/01/25-13:56:01.275769 7f716b9939c0 Options.compaction_style: level
2017/01/25-13:56:01.275770 7f716b9939c0 Options.compaction_pri: by compensated size

@facebook-github-bot
Copy link
Contributor

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

@@ -688,6 +688,18 @@ static std::unordered_map<std::string, CompactionStyle>
{"kCompactionStyleFIFO", kCompactionStyleFIFO},
{"kCompactionStyleNone", kCompactionStyleNone}};

static std::unordered_map<char, std::string> compaction_style_logging_map = {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change char to CompactionStyle

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a name like compaction_style_to_string

Copy link
Author

Choose a reason for hiding this comment

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

unordered_map does not support enum as a key well. Unless we have something like: (http://stackoverflow.com/questions/18837857/cant-use-enum-class-as-unordered-map-key). Is that what we want?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we use std::map ?

Copy link
Author

Choose a reason for hiding this comment

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

sure, given the size is small :D

@@ -688,6 +688,18 @@ static std::unordered_map<std::string, CompactionStyle>
{"kCompactionStyleFIFO", kCompactionStyleFIFO},
{"kCompactionStyleNone", kCompactionStyleNone}};

static std::unordered_map<char, std::string> compaction_style_logging_map = {
{kCompactionStyleLevel, "level"},
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use the exact name of the enum as the string when logging
kCompactionStyleLevel instead of level
and so one

{kCompactionStyleFIFO, "fifo"},
{kCompactionStyleNone, "none"}};

static std::unordered_map<char, std::string> compaction_pri_logging_map = {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

{kCompactionStyleNone, "none"}};

static std::unordered_map<char, std::string> compaction_pri_logging_map = {
{kByCompensatedSize, "by compensated size"},
Copy link
Contributor

Choose a reason for hiding this comment

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

same kByCompensatedSize instead of by compensated size

@facebook-github-bot
Copy link
Contributor

@highker updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

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

@IslamAbdelRahman
Copy link
Contributor

LGTM, but looks like there is an issue in importing the diff

@highker
Copy link
Author

highker commented Jan 30, 2017

Command 'cd '/data/sandcastle/boxes/instance-rocksdb-int-git-convert-pull-request-to-internal-diff' && GIT_COMMITTER_EMAIL=svcscm@fb.com GIT_COMMITTER_NAME='Service User' git commit --amend '--author=svcscm' '-m' 'temporary commit'' failed with exit code

stderr:
fatal: --author 'svcscm' is not 'Name ' and matches no existing author
' in /var/releases/continuous_www_scripts3/flib/intern/sandcastle/shell/SandcastleStatusDecoderBase.php:87

Did I miss some config?

@IslamAbdelRahman
Copy link
Contributor

@highker, can you please rebase your changes ?

@facebook-github-bot
Copy link
Contributor

@highker updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@highker updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@highker updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@highker updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@highker updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@highker updated the pull request - view changes - changes since last import

…ring

Summary:
Replace the numerical output for compaction_style and compaction_pri
with strings

Test Plan:
Manually tested:
before:
2017/01/25-13:17:42.948086 7f09a3da59c0 Options.compaction_style: 0
2017/01/25-13:17:42.948087 7f09a3da59c0   Options.compaction_pri: 0
after:
2017/01/25-13:56:01.275769 7f716b9939c0 Options.compaction_style: level
2017/01/25-13:56:01.275770 7f716b9939c0   Options.compaction_pri: by compensated size

Reviewers: sdong

Subscribers: nathro

Differential Revision: https://phabricator.intern.facebook.com/D4464638

Tasks: 11090832
@facebook-github-bot
Copy link
Contributor

@highker updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

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

@highker highker self-assigned this Feb 7, 2017
@highker highker deleted the my_new_feature branch February 7, 2017 19:10
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