[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot

[GitBox <gi...@apache.org>]

anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1064647933


##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java:
##########
@@ -1151,6 +1213,9 @@ protected void setLocalSessionFlag(Request si) {
     }
 
     public void submitRequest(Request si) {
+        if (state == State.MAINTENANCE) {
+            throw new IllegalStateException("Zookeeper server is in maintenance state");
+        }

Review Comment:
   If you throw an exception here, the request will fail on the client side. I haven't tried it myself, have you valdiated that the client is able to seemlessly retry the command without bothering the user?
   If not, I suggest another solution: create a semaphore which is closed while the maintenace is happening and block this thread until it's finished. The request will suffer some additionaly latency, but otherwise cannot be noticed. Thoughts?



##########
zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md:
##########
@@ -2126,6 +2126,17 @@ options are used to configure the [AdminServer](#sc_adminserver).
   The time interval for rate limiting snapshot command to protect the server.
   Defaults to 5 mins.
 
+* *admin.restore.enabled* :
+  (Java system property: **zookeeper.admin.restore.enabled**)
+  The flag for enabling the restore command. Defaults to false.
+  It will be enabled by default once the auth support for admin server commands
+  is available.
+
+* *admin.restore.intervalInMS* :
+  (Java system property: **zookeeper.admin.restore.intervalInMS**)
+  The time interval for rate limiting restore command to protect the server.
+  Defaults to 5 mins.

Review Comment:
   Do we need this setting separately for all admin commands?
   Shouldn't we just introduce a new general setting for the admin interface which would rate limit all admin requests coming in? 
   I don't think anybody would like to set this individually for commands.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org

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