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

Add configuration files and configuration classes #387

Merged
merged 15 commits into from
Dec 28, 2019
Merged

Add configuration files and configuration classes #387

merged 15 commits into from
Dec 28, 2019

Conversation

kairbon
Copy link
Collaborator

@kairbon kairbon commented Dec 24, 2019 •
edited
Loading

Generally speaking, every large project needs a configuration file for users to configure, so this PR solves the problem by typesafe

@kairbon kairbon changed the title Add master-slave synchronization [WIP] Add master-slave synchronization Dec 24, 2019
@kairbon kairbon changed the title [WIP] Add master-slave synchronization [WIP] Add configuration files and configuration classes Dec 24, 2019
@kairbon kairbon requested a review from jovany-wang December 25, 2019 15:02
@kairbon kairbon changed the title [WIP] Add configuration files and configuration classes Add configuration files and configuration classes Dec 25, 2019
jovany-wang
jovany-wang requested changes Dec 25, 2019
View reviewed changes
Copy link
Collaborator

@jovany-wang jovany-wang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  1. No need to use the config items everywhere, use config.getXXX() instead.
  2. Do not set the config items one by one.

BTW, it's better to add a brief about what you changed in this PR.

server/src/main/resources/dst_server.conf Outdated Show resolved Hide resolved
server/src/main/java/com/distkv/dst/server/DstServerConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/distkv/dst/server/DstServer.java Outdated Show resolved Hide resolved
server/src/main/java/com/distkv/dst/server/DstServer.java Outdated Show resolved Hide resolved
@jovany-wang jovany-wang added the enhancement enhancement label Dec 25, 2019
jovany-wang
jovany-wang reviewed Dec 28, 2019
View reviewed changes
Copy link
Collaborator

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

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

Looks good overall. I left some minor comments ~

server/src/main/resources/Dst.default.conf Outdated Show resolved Hide resolved
server/src/main/java/com/distkv/dst/server/DstServerConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/distkv/dst/server/DstServerConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/distkv/dst/server/DstServerConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/distkv/dst/server/DstServerConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/distkv/dst/server/DstServerConfig.java Outdated Show resolved Hide resolved
jovany-wang
jovany-wang approved these changes Dec 28, 2019
View reviewed changes
Copy link
Collaborator

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

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

Nice job.

Thanks for your contributions.

@jovany-wang jovany-wang merged commit 5204ae0 into distkv-project:master Dec 28, 2019
@jovany-wang jovany-wang mentioned this pull request Dec 29, 2019
Closed
@kairbon kairbon deleted the addMaster branch March 23, 2020 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jovany-wang jovany-wang jovany-wang approved these changes

Assignees
No one assigned
Labels
enhancement enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kairbon @jovany-wang