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

feat: dynamic reload duplication config #2103

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ninsmiracle
Copy link
Contributor

What problem does this PR solve?

#2102

What is changed and how does it work?

If the cluster maintainer push new config to target cluster, you can use pegasus-shell to make cluster reload new duplication config without nodes restart.
I add some function to help servers get their config on time, and we can reload new config ONLY about duplication.

Tests
  • Unit test
  • Manual test (add detailed scripts or steps below)
  • Cluster test
Code changes
  • Has exported function/method change
    Move and copy constructors of configuration class.
    Some function about reloading dup parameter into memory instance.
Related changes
  • Need to update the user documentation
  • Need to be included in the release note

@ninsmiracle
Copy link
Contributor Author

How to use?

  1. Push new config file (config.ini) with your own method.
  2. Open pegasus-shell and type remote_command -t all dup-config-reload
  3. Begin duplication as normal. Just like: dup add -c c3tst-performance3 -p

What can it do?

This PR can ONLY reload the parameter about duplication, so that it will be safety.

[[pegasus.clusters]]
   %{cluster.name} = %{meta_address}
    # duplication clusters already have
   c4tst-master = 10.xxx.xx.xx:22601,10.xxx.xx.x:22601
   c3tst-backup = 10.xxx.xx.xx:37001,10.xxx.xx.x:37001
   # If you want to add another cluster in this duplication situation
   xxxxx = xxxxxxxxxxxx
   
  [[duplication-group]]
  c4tst-master = 1 
  c3tst-backup = 2 
  xxxx = 3

src/common/duplication_common.cpp Outdated Show resolved Hide resolved
src/common/duplication_common.cpp Outdated Show resolved Hide resolved
src/common/duplication_common.cpp Outdated Show resolved Hide resolved
@@ -0,0 +1,106 @@
; The MIT License (MIT)
Copy link
Member

Choose a reason for hiding this comment

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

Use Apache 2.0 license.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a config file that I copy the file header of src/common/test/config-test.ini. And I don't know how to write the copyright owner of this config file.

Copyright [yyyy] [name of copyright owner]

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

std::string value;
int line;

bool present;
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add some comments when you happened to modify the code? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At begin I declaring configuration(config_map&) to use the mutex lock the map. If I not move private parameter before public function, an error will be reported during the compilation phase: the variable cannot be found.

@@ -51,6 +51,20 @@ configuration::~configuration()
_configs.clear();
}

void configuration::copy_configs(configuration &source_conf)
{
source_conf._lock.lock();
Copy link
Member

Choose a reason for hiding this comment

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

_configs should be empty before filling values, right? Please check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I call configuration::copy_configs, it will call configuration::clear_configs to make _configs clearly.

src/utils/configuration.cpp Outdated Show resolved Hide resolved
*old_config = g_config;
dsn::configuration temp_config;
if (!temp_config.load(file, arguments)) {
// todo gns: add some error log
Copy link
Member

Choose a reason for hiding this comment

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

Use the standard TODO style.

@acelyc111
Copy link
Member

Thanks for the contribution! Before this pull request, let's discuss clearly the requirement in #2102.

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.

2 participants