acelyc111 commented on a change in pull request #4704:
URL: https://github.com/apache/incubator-doris/pull/4704#discussion_r502145777



##########
File path: be/src/common/configbase.cpp
##########
@@ -227,6 +231,34 @@ bool Properties::get(const char* key, const char* defstr, 
T& retval) const {
     return strtox(valstr, retval);
 }
 
+void Properties::set(const std::string& key, const std::string& val) {
+    file_conf_map.emplace(key, val);
+}
+
+bool Properties::dump(const std::string& conffile) {
+    std::vector<std::string> files = { conffile };
+    Status st = FileSystemUtil::remove_paths(files); 
+    if (!st.ok()) {
+        return false;
+    }
+    st = FileSystemUtil::create_file(conffile);
+    if (!st.ok()) {
+        return false;
+    }
+
+    std::ofstream out(conffile);
+    out << "# THIS IS AN AUTO GENERATED CONFIG FILE.\n";
+    out << "# You can modify this file manually, and the configurations in 
this file\n";
+    out << "# will overwrite the configurations in fe.conf\n";

Review comment:
       ```suggestion
       out << "# will overwrite the configurations in be.conf\n";
   ```

##########
File path: docs/en/administrator-guide/config/be_config.md
##########
@@ -30,51 +30,70 @@ under the License.
 
 This document mainly introduces the relevant configuration items of BE.
 
+The BE configuration file `be.conf` is usually stored in the `conf/` directory 
of the BE deployment path. In version 0.14, another configuration file 
`be_custom.conf` will be introduced. The configuration file is used to record 
the configuration items that are dynamically configured and persisted by the 
user during operation.
+
+After the BE process is started, it will read the configuration items in 
`be.conf` first, and then read the configuration items in `be_custom.conf`. The 
configuration items in `be_custom.conf` will overwrite the same configuration 
items in `be.conf`.
+
 ## View configuration items
-(TODO)
+
+Users can view the current configuration items by visiting BE's web page:
+
+`http://be_host:be_webserver_port/varz`
 
 ## Set configuration items
 
 There are two ways to configure BE configuration items:
 
 1. Static configuration
 
-         Add and set configuration items in the `conf/be.conf` file. The 
configuration items in `be.conf` will be read when BE starts. Configuration 
items not in `be.conf` will use default values.
+    Add and set configuration items in the `conf/be.conf` file. The 
configuration items in `be.conf` will be read when BE starts. Configuration 
items not in `be.conf` will use default values.
 
 2. Dynamic configuration
 
-         After BE starts, the configuration items can be dynamically set with 
the following commands.
+    After BE starts, the configuration items can be dynamically set with the 
following commands.
+
+    ```
+    curl -X POST http://{be_ip}:{be_http_port}/api/update_config?{key}={value}'
+    ```
 
-         ```curl -X POST 
http://{be_ip}:{be_http_port}/api/update_config?{key}={value}'```
+    In version 0.13 and before, the configuration items modified in this way 
will become invalid after the BE process restarts. In 0.14 and later versions, 
the modified configuration can be persisted through the following command. The 
modified configuration items are stored in the `be_custom.conf` file.
 
-         **Configuration items modified in this way will become invalid after 
the BE process restarts. **
+    ```
+    curl -X POST 
http://{be_ip}:{be_http_port}/api/update_config?{key}={value}&persis=true'

Review comment:
       ```suggestion
       curl -X POST 
http://{be_ip}:{be_http_port}/api/update_config?{key}={value}&persist=true'
   ```

##########
File path: docs/en/administrator-guide/config/be_config.md
##########
@@ -30,51 +30,70 @@ under the License.
 
 This document mainly introduces the relevant configuration items of BE.
 
+The BE configuration file `be.conf` is usually stored in the `conf/` directory 
of the BE deployment path. In version 0.14, another configuration file 
`be_custom.conf` will be introduced. The configuration file is used to record 
the configuration items that are dynamically configured and persisted by the 
user during operation.

Review comment:
       ... is used for recording ... ?

##########
File path: be/src/http/action/update_config_action.cpp
##########
@@ -38,25 +38,53 @@ namespace doris {
 
 const static std::string HEADER_JSON = "application/json";
 
+const static std::string PERSIST_PARAM = "persist";
+
 void UpdateConfigAction::handle(HttpRequest* req) {
     LOG(INFO) << req->debug_string();
 
     Status s;
     std::string msg;
-    if (req->params()->size() != 1) {
+    // We only support set one config at a time, and along with a optional 
param "persist".
+    // So the number of query params should at most be 2.
+    if (req->params()->size() > 2 || req->params()->size() < 1) {
         s = Status::InvalidArgument("");
         msg = "Now only support to set a single config once, via 
'config_name=new_value'";

Review comment:
       I think you should also describe the optional parameter `persist` in 
error message.

##########
File path: be/src/http/action/update_config_action.cpp
##########
@@ -38,25 +38,53 @@ namespace doris {
 
 const static std::string HEADER_JSON = "application/json";
 
+const static std::string PERSIST_PARAM = "persist";
+
 void UpdateConfigAction::handle(HttpRequest* req) {
     LOG(INFO) << req->debug_string();
 
     Status s;
     std::string msg;
-    if (req->params()->size() != 1) {
+    // We only support set one config at a time, and along with a optional 
param "persist".
+    // So the number of query params should at most be 2.
+    if (req->params()->size() > 2 || req->params()->size() < 1) {
         s = Status::InvalidArgument("");
         msg = "Now only support to set a single config once, via 
'config_name=new_value'";
     } else {
-        DCHECK(req->params()->size() == 1);
-        const std::string& config = req->params()->begin()->first;
-        const std::string& new_value = req->params()->begin()->second;
-        s = config::set_config(config, new_value);
-        if (s.ok()) {
-            LOG(INFO) << "set_config " << config << "=" << new_value << " 
success";
-        } else {
-            LOG(WARNING) << "set_config " << config << "=" << new_value << " 
failed";
-            msg = strings::Substitute("set $0=$1 failed, reason: $2", config, 
new_value,
-                                      s.to_string());
+        if (req->params()->size() == 1) {
+            const std::string& config = req->params()->begin()->first;
+            const std::string& new_value = req->params()->begin()->second;
+            s = config::set_config(config, new_value, false);
+            if (s.ok()) {
+                LOG(INFO) << "set_config " << config << "=" << new_value << " 
success";
+            } else {
+                LOG(WARNING) << "set_config " << config << "=" << new_value << 
" failed";
+                msg = strings::Substitute("set $0=$1 failed, reason: $2", 
config, new_value,
+                        s.to_string());
+            }
+        } else if (req->params()->size() == 2) {
+            if (req->params()->find(PERSIST_PARAM) == req->params()->end()) {
+                s = Status::InvalidArgument("");
+                msg = "Now only support to set a single config once, via 
'config_name=new_value'";

Review comment:
       Same




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org

Reply via email to