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

Support CLI options for kvrocks server #1037

Merged
merged 5 commits into from
Oct 24, 2022
Merged

Support CLI options for kvrocks server #1037

merged 5 commits into from
Oct 24, 2022

Conversation

PragmaTwice
Copy link
Member

@PragmaTwice PragmaTwice commented Oct 24, 2022 •
edited
Loading

It closes #1034

@git-hulk
Copy link
Member

wow, so speedy.

tisonkun
tisonkun requested changes Oct 24, 2022
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

We'd better guard config keys with a namespace.

tests/gocase/integration/cli_options/cli_options_test.go Show resolved Hide resolved
tisonkun
tisonkun requested changes Oct 24, 2022
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

OK. Then I have no objection on the option format. More comments inline.

src/main.cc Outdated Show resolved Hide resolved
@PragmaTwice PragmaTwice requested a review from tisonkun October 24, 2022 12:38
tisonkun
tisonkun requested changes Oct 24, 2022
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

I test with ./build/kvrocks --unknown k and it starts well. I'm thinking of whether we should fail if the config key is unknown.

src/main.cc Show resolved Hide resolved
src/config/config.h
@@ -61,6 +61,14 @@ struct CompactionCheckerRange {
bool Enabled() { return Start != -1 || Stop != -1; }
};

struct CLIOptions {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd prefer as simple as Options.

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 think Options is not specific enough.

@PragmaTwice
Copy link
Member Author

PragmaTwice commented Oct 24, 2022 •
edited
Loading

I test with ./build/kvrocks --unknown k and it starts well. I'm thinking of whether we should fail if the config key is unknown.

If you put an unkown k in the config file, it will also start well. It is not a problem of this PR.

@PragmaTwice PragmaTwice added the feature type new feature label Oct 24, 2022
@PragmaTwice
Copy link
Member Author

Thanks all. Merging...

@PragmaTwice PragmaTwice merged commit c09cb8c into apache:unstable Oct 24, 2022
@tisonkun tisonkun mentioned this pull request Nov 24, 2022
Merged
@git-hulk git-hulk mentioned this pull request Feb 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanruixiang tanruixiang tanruixiang left review comments

@tisonkun tisonkun tisonkun approved these changes

@git-hulk git-hulk git-hulk approved these changes

@torwig torwig Awaiting requested review from torwig

@ShooterIT ShooterIT Awaiting requested review from ShooterIT

@caipengbo caipengbo Awaiting requested review from caipengbo

Assignees
No one assigned
Labels
feature type new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for CLI arguments/environment variables to overwrite conf file values
5 participants
@PragmaTwice @git-hulk @torwig @tisonkun @tanruixiang