You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/12/13 07:19:34 UTC
[GitHub] [zookeeper] li4wang opened a new pull request, #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
li4wang opened a new pull request, #1961:
URL: https://github.com/apache/zookeeper/pull/1961
Provides a restore command for restoring database from a snapshot
Author: Li Wang <li...@apple.com>
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1070184620
##########
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:
> Add MAINTENANCE as a running state to ZooKeeperServer.isRunning() method.
Adding MAINTENANCE to ZooKeeperServer.isRunning() will change the semantics of "running" state and has bigger impact, we would want to avoid it if it's possible.
> Do not alter the state of ZooKeeperServer, but use a separate volatile variable in this class to block requests temporarly.
Good idea. Yes, we don't have to alter the state of ZooKeeper if we don't think we need a generic MAINTENANCE state.
Instead of using volatile variable, how about adding a `CountDownLatch` to more efficiently coordinate/sync between threads and utilize system resources.
The `restoreFromSnapshot()` API will instantiate a `CountDownLatch` with 1 count when restore starts and count it down when restore is completed. The `submitRequest()` API will check and wait on the `CountDownLatch`, so requests will be blocked if a restore is in progress.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1070177732
##########
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:
Thanks for the feedback and inputs, @eolivelli and @anmolnar
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1066057476
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java:
##########
@@ -583,6 +590,58 @@ public synchronized File takeSnapshot(boolean syncSnap, boolean isSevere, boolea
return snapFile;
}
+ /**
+ * Restores database from a snapshot. It is used by the restore admin server command.
+ *
+ * @param inputStream input stream of snapshot
+ * @Return last processed zxid
+ * @throws IOException
+ */
+ public synchronized long restoreFromSnapshot(final InputStream inputStream) throws IOException {
+ if (inputStream == null) {
+ throw new IllegalArgumentException("InputStream can not be null when restoring from snapshot");
+ }
+
+ long start = Time.currentElapsedTime();
+ LOG.info("Before restore database. lastProcessedZxid={}, nodeCount={}，sessionCount={}",
+ getZKDatabase().getDataTreeLastProcessedZxid(),
+ getZKDatabase().dataTree.getNodeCount(),
+ getZKDatabase().getSessionCount());
+
+ // restore to a new zkDatabase
+ final ZKDatabase newZKDatabase = new ZKDatabase(this.txnLogFactory);
+ final CheckedInputStream cis = new CheckedInputStream(new BufferedInputStream(inputStream), new Adler32());
+ final InputArchive ia = BinaryInputArchive.getArchive(cis);
+ newZKDatabase.deserializeSnapshot(ia, cis);
+ LOG.info("Restored to a new database. lastProcessedZxid={}, nodeCount={}, sessionCount={}",
+ newZKDatabase.getDataTreeLastProcessedZxid(),
+ newZKDatabase.dataTree.getNodeCount(),
+ newZKDatabase.getSessionCount());
+
+ // set the state to MAINTENANCE to stop taking incoming requests
+ setState(State.MAINTENANCE);
Review Comment:
I don't think we have to `notifyAll()` here, because there shouldn't be any thread to release while doing maintenance.
--
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
[GitHub] [zookeeper] li4wang commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1347869908
This is the PR for restore command, which is the second patch of backup and restore feature.
Since the snapshot PR https://github.com/apache/zookeeper/pull/1943 is currently blocked by the CI build issue, this PR is created on top of the snapshot and contains two commits. One is for restore and one is for snapshot.
--
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
[GitHub] [zookeeper] eolivelli commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1377283397
@li4wang can you please put some links in the description of this PR about how the administrator should operate a "restore" ?
A ZooKeeper cluster is made of peers and you cannot "restore" the database only on one peer as it will be out of sync with the rest of the cluster.
We should provide a guide to correctly operate this operation.
--
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
[GitHub] [zookeeper] eolivelli commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1400388061
@li4wang CI is failing
```
[WARNING] Files with unapproved licenses:
/home/runner/work/zookeeper/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/ZookeeperServerRestoreTest.java
```
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1070184620
##########
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:
> Add MAINTENANCE as a running state to ZooKeeperServer.isRunning() method.
Adding MAINTENANCE to ZooKeeperServer.isRunning() will change the semantics of "running" state and has bigger impact, we would want to avoid it if it's possible.
> Do not alter the state of ZooKeeperServer, but use a separate volatile variable in this class to block requests temporarly.
Good idea. Yes, we don't have to alter the state of ZooKeeper if we don't think we need a generic MAINTENANCE state.
Instead of using volatile variable, how about adding a `CountDownLatch` to more efficiently coordinate/sync between threads and utilize system resources?
The `restoreFromSnapshot()` API will instantiate a `CountDownLatch` with 1 count when restore starts and count it down when restore is completed. The `submitRequest()` API will check and wait on the `CountDownLatch`, so requests will be blocked if a restore is in progress.
--
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
[GitHub] [zookeeper] li4wang commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1405315042
> One new test failed, PTAL
The test passed. Thanks
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1070184620
##########
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:
> Add MAINTENANCE as a running state to ZooKeeperServer.isRunning() method.
Adding MAINTENANCE to ZooKeeperServer.isRunning() will change the semantics of "running" state and has bigger impact, we would want to avoid it if it's possible.
> Do not alter the state of ZooKeeperServer, but use a separate volatile variable in this class to block requests temporarly.
Good idea. Yes, we don't have to alter the state of ZooKeeper if we don't think we need a generic MAINTENANCE state.
Instead of using volatile variable, We can add a `CountDownLatch` to more efficiently coordinate/sync between threads and utilize system resources.
The `restoreFromSnapshot()` API will instantiate a `restoreLatch` with 1 count when starting the restore and count it down when the restore is completed. The `submitRequest()` API will check and wait on the `restoreLatch`.
--
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
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1048884064
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -259,15 +261,81 @@ protected void doGet(
}
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null);
+ response.setStatus(cmdResponse.getStatusCode());
- // Format and print the output of the command
- CommandOutputter outputter = new JsonOutputter();
- response.setStatus(HttpServletResponse.SC_OK);
+ final Map<String, String> headers = cmdResponse.getHeaders();
+ for (final Map.Entry<String, String> header : headers.entrySet()) {
+ response.addHeader(header.getKey(), header.getValue());
+ }
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ if (cmdResponse.getInputStream() == null) {
+ // Format and print the output of the command
+ CommandOutputter outputter = new JsonOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getWriter());
+ } else {
+ // Stream out the output of the command
+ CommandOutputter outputter = new StreamOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getOutputStream());
+ }
+ }
+
+ /**
+ * Serves HTTP POST requests. It reads request payload as raw data.
+ * It's up to each command to process the payload accordingly.
+ * For example, RestoreCommand uses the payload InputStream directly
+ * to read snapshot data.
+ */
+ @Override
+ protected void doPost(final HttpServletRequest request,
+ final HttpServletResponse response) throws ServletException, IOException {
+ final String cmdName = extractCommandNameFromURL(request, response);
+ if (cmdName != null) {
+ // Run the command
+ final CommandResponse cmdResponse = Commands.runCommand(cmdName, zkServer, null, request.getInputStream());
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ sendJSONResponse(response, cmdResponse, clientIP);
+ }
+ }
+
+ /**
+ * Extracts the command name from URL if it exists otherwise null
+ */
+ private String extractCommandNameFromURL(final HttpServletRequest request,
+ final HttpServletResponse response) throws IOException {
+ String cmd = request.getPathInfo();
+ if (cmd == null || cmd.equals("/")) {
+ printCommandLinks(response);
+ return null;
+ }
+ // Strip leading "/"
+ return cmd.substring(1);
+ }
+
+ /**
+ * Prints the list of URLs to each registered command as response.
+ */
+ private void printCommandLinks(final HttpServletResponse response) throws IOException {
+ for (final String link : commandLinks()) {
+ response.getWriter().println(link);
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1048113591
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -620,6 +623,48 @@ public void deserializeSnapshot(InputArchive ia) throws IOException {
initialized = true;
}
+ /**
+ * Deserialize a snapshot that contains FileHeader from an input archive. It is used by
+ * the admin restore command.
+ *
+ * @param ia the input archive to deserialize from
+ * @param is the CheckInputStream to check integrity
+ *
+ * @throws IOException
+ */
+ public synchronized void deserializeSnapshot(final InputArchive ia, final CheckedInputStream is) throws IOException {
+ // clear the zkDatabase
+ clear();
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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
[GitHub] [zookeeper] li4wang commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1377558665
> We should provide a guide to correctly operate this operation.
Good point. Will do. Thanks
--
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
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1067966677
##########
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:
Makes sense to me.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1070184620
##########
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:
> Add MAINTENANCE as a running state to ZooKeeperServer.isRunning() method.
Adding MAINTENANCE to ZooKeeperServer.isRunning() will change the semantics of "running" state and has bigger impact, we would want to avoid it if it's possible.
> Do not alter the state of ZooKeeperServer, but use a separate volatile variable in this class to block requests temporarly.
Good idea. Yes, we don't have to alter the state of ZooKeeper if we don't think we need a generic MAINTENANCE state.
Instead of using volatile variable, We can add a `CountDownLatch` to more efficiently coordinate/sync between threads and utilize system resources.
The `restoreFromSnapshot()` API will instantiate a `CountDownLatch` with 1 count when restore starts and count it down when restore is completed. The `submitRequest()` API will check and wait on the `CountDownLatch`, so requests will be blocked if a restore is in progress.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1070184620
##########
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:
> Add MAINTENANCE as a running state to ZooKeeperServer.isRunning() method.
Adding MAINTENANCE to ZooKeeperServer.isRunning() will change the semantics of "running" state and has bigger impact, we would want to avoid it if it's possible.
> Do not alter the state of ZooKeeperServer, but use a separate volatile variable in this class to block requests temporarly.
Good idea. Yes, we don't have to alter the state of ZooKeeper if we don't think we need a generic MAINTENANCE state.
Instead of using volatile variable, We can add a `CountDownLatch` to more efficiently coordinate/sync between threads and utilize system resources.
The `restoreFromSnapshot()` API will instantiate a `CountDownLatch` with 1 count when starting the restore and count it down when the restore is completed. The `submitRequest()` API will check and wait on the `CountDownLatch`.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1070184620
##########
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:
> Add MAINTENANCE as a running state to ZooKeeperServer.isRunning() method.
Adding MAINTENANCE to ZooKeeperServer.isRunning() will change the semantics of "running" state and has bigger impact, we would want to avoid it if it's possible.
> Do not alter the state of ZooKeeperServer, but use a separate volatile variable in this class to block requests temporarly.
Good idea. Yes, we don't have to alter the state of ZooKeeper if we don't think we need a generic MAINTENANCE state.
Instead of using volatile variable, I will add a `CountDownLatch` to efficiently coordinate/sync between threads and utilize system resources. I will update the PR with the changes shortly.
--
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
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1071336270
##########
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:
Makes sense to me.
Maybe before starting to way you can log at INFO level a line like "MAINTEINANCE in progress, blocking the request processing"
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1048113099
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -620,6 +623,48 @@ public void deserializeSnapshot(InputArchive ia) throws IOException {
initialized = true;
}
+ /**
+ * Deserialize a snapshot that contains FileHeader from an input archive. It is used by
+ * the admin restore command.
+ *
+ * @param ia the input archive to deserialize from
+ * @param is the CheckInputStream to check integrity
+ *
+ * @throws IOException
+ */
+ public synchronized void deserializeSnapshot(final InputArchive ia, final CheckedInputStream is) throws IOException {
+ // clear the zkDatabase
+ clear();
Review Comment:
@sonatype-lift ignore
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1048884022
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -259,15 +261,81 @@ protected void doGet(
}
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null);
+ response.setStatus(cmdResponse.getStatusCode());
- // Format and print the output of the command
- CommandOutputter outputter = new JsonOutputter();
- response.setStatus(HttpServletResponse.SC_OK);
+ final Map<String, String> headers = cmdResponse.getHeaders();
+ for (final Map.Entry<String, String> header : headers.entrySet()) {
+ response.addHeader(header.getKey(), header.getValue());
+ }
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ if (cmdResponse.getInputStream() == null) {
+ // Format and print the output of the command
+ CommandOutputter outputter = new JsonOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getWriter());
+ } else {
+ // Stream out the output of the command
+ CommandOutputter outputter = new StreamOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getOutputStream());
+ }
+ }
+
+ /**
+ * Serves HTTP POST requests. It reads request payload as raw data.
+ * It's up to each command to process the payload accordingly.
+ * For example, RestoreCommand uses the payload InputStream directly
+ * to read snapshot data.
+ */
+ @Override
+ protected void doPost(final HttpServletRequest request,
+ final HttpServletResponse response) throws ServletException, IOException {
+ final String cmdName = extractCommandNameFromURL(request, response);
+ if (cmdName != null) {
+ // Run the command
+ final CommandResponse cmdResponse = Commands.runCommand(cmdName, zkServer, null, request.getInputStream());
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ sendJSONResponse(response, cmdResponse, clientIP);
+ }
+ }
+
+ /**
+ * Extracts the command name from URL if it exists otherwise null
+ */
+ private String extractCommandNameFromURL(final HttpServletRequest request,
+ final HttpServletResponse response) throws IOException {
+ String cmd = request.getPathInfo();
+ if (cmd == null || cmd.equals("/")) {
+ printCommandLinks(response);
+ return null;
+ }
+ // Strip leading "/"
+ return cmd.substring(1);
+ }
+
+ /**
+ * Prints the list of URLs to each registered command as response.
+ */
+ private void printCommandLinks(final HttpServletResponse response) throws IOException {
+ for (final String link : commandLinks()) {
+ response.getWriter().println(link);
Review Comment:
@sonatype-lift ignore
--
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
[GitHub] [zookeeper] li4wang commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1352127568
@eolivelli @symat @anmolnar would you mind reviewing the PR? Thanks
--
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
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1064790569
##########
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:
blocking here is probably not a good idea, we risk to block the pipeline, with unpredictable consequences probably
--
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
[GitHub] [zookeeper] li4wang commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1376518428
> can you please fix the file ?
Thanks for adding the license header @eolivelli . Sorry that I didn't see the comment in time.
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by 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
[GitHub] [zookeeper] eolivelli commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1373344882
@li4wang license check (RAT) is failing
```
[INFO] 30 explicit excludes (use -debug for more details).
[INFO] 816 resources included (use -debug for more details)
[WARNING] Files with unapproved licenses:
/home/runner/work/zookeeper/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/ZookeeperServerRestoreTest.java
```
can you please fix the file ?
--
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
[GitHub] [zookeeper] eolivelli commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1403214322
One new test failed, PTAL
[INFO]
[ERROR] Errors:
[ERROR] SnapshotAndRestoreCommandTest.testClientRequest_restoreInProgress:207 » NoNode
[INFO]
[ERROR] Tests run: 3007, Failures:
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1065872646
##########
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:
What are you thinking of exactly?
We already block the thread when RequestThrottler or first processor is not yet initialized with the following logic:
```java
while (state == State.INITIAL) {
wait(1000);
}
```
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1065216167
##########
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:
I don't see a real need to configure the interval differently for the snapshot and restore command at this point.
Yes, we can keep it simple and have just one general setting for all the admin commands that need to be rate limited.
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1066055084
##########
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:
Yeah, be aware that you have to do `notifyAll()` after changing the state back to RUNNING to notify the waiting threads. I'll add another comment at the right place.
Additionally, I don't get this `check-wait(timeout)-check-wait(timeout)-...` logic. It doesn't make sense to me, there's no additional fail-safe logic inside the loop, why don't just `wait()`? Instead, it releases the lock in every second probably without any benefit. I don't say we have to change that, I just don't understand.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1067613912
##########
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?
I checked the scenario of sending client request while zk is in maintenance state. Here is what I found.
1. Connection/session creation will be retried but read/write operation will not.
2. The read/write request will fail even no exception is thrown here, because the request fails before sending to ZookeeperServer. Here is what happens:
a) `ServerCnxn` detects that ZK server is not running and close the socket/channel.
b) `ClientSocketCnxn.doTransport()` then detects that socket/channel is closed and throws IOException
c) 'SendThread' catches the Exception, adds `ConnectionLoss` error reply header, clears up the outgoingQueue ann pendingQueue, and attempts to re-connect to server
d) Zookeeper check the header and sends ConnectionLoss to user
--
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
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1046760931
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##########
@@ -259,15 +261,81 @@ protected void doGet(
}
// Run the command
- CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs);
+ final CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, kwargs, null);
+ response.setStatus(cmdResponse.getStatusCode());
- // Format and print the output of the command
- CommandOutputter outputter = new JsonOutputter();
- response.setStatus(HttpServletResponse.SC_OK);
+ final Map<String, String> headers = cmdResponse.getHeaders();
+ for (final Map.Entry<String, String> header : headers.entrySet()) {
+ response.addHeader(header.getKey(), header.getValue());
+ }
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ if (cmdResponse.getInputStream() == null) {
+ // Format and print the output of the command
+ CommandOutputter outputter = new JsonOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getWriter());
+ } else {
+ // Stream out the output of the command
+ CommandOutputter outputter = new StreamOutputter(clientIP);
+ response.setContentType(outputter.getContentType());
+ outputter.output(cmdResponse, response.getOutputStream());
+ }
+ }
+
+ /**
+ * Serves HTTP POST requests. It reads request payload as raw data.
+ * It's up to each command to process the payload accordingly.
+ * For example, RestoreCommand uses the payload InputStream directly
+ * to read snapshot data.
+ */
+ @Override
+ protected void doPost(final HttpServletRequest request,
+ final HttpServletResponse response) throws ServletException, IOException {
+ final String cmdName = extractCommandNameFromURL(request, response);
+ if (cmdName != null) {
+ // Run the command
+ final CommandResponse cmdResponse = Commands.runCommand(cmdName, zkServer, null, request.getInputStream());
+ final String clientIP = IPAuthenticationProvider.getClientIPAddress(request);
+ sendJSONResponse(response, cmdResponse, clientIP);
+ }
+ }
+
+ /**
+ * Extracts the command name from URL if it exists otherwise null
+ */
+ private String extractCommandNameFromURL(final HttpServletRequest request,
+ final HttpServletResponse response) throws IOException {
+ String cmd = request.getPathInfo();
+ if (cmd == null || cmd.equals("/")) {
+ printCommandLinks(response);
+ return null;
+ }
+ // Strip leading "/"
+ return cmd.substring(1);
+ }
+
+ /**
+ * Prints the list of URLs to each registered command as response.
+ */
+ private void printCommandLinks(final HttpServletResponse response) throws IOException {
+ for (final String link : commandLinks()) {
+ response.getWriter().println(link);
Review Comment:
*[XSS_SERVLET](https://find-sec-bugs.github.io/bugs.htm#XSS_SERVLET):* This use of java/io/PrintWriter.println(Ljava/lang/String;)V could be vulnerable to XSS in the Servlet
---
<details><summary><b>ℹ️ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=360445073&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=360445073&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=360445073&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=360445073&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=360445073&lift_comment_rating=5) ]
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -620,6 +623,48 @@ public void deserializeSnapshot(InputArchive ia) throws IOException {
initialized = true;
}
+ /**
+ * Deserialize a snapshot that contains FileHeader from an input archive. It is used by
+ * the admin restore command.
+ *
+ * @param ia the input archive to deserialize from
+ * @param is the CheckInputStream to check integrity
+ *
+ * @throws IOException
+ */
+ public void deserializeSnapshot(final InputArchive ia, final CheckedInputStream is) throws IOException {
+ // clear the zkDatabase
+ clear();
Review Comment:
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `ZKDatabase.deserializeSnapshot(...)` indirectly writes to field `this.dataTree` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>ℹ️ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=360445341&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=360445341&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=360445341&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=360445341&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=360445341&lift_comment_rating=5) ]
--
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
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1048113172
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -620,6 +623,48 @@ public void deserializeSnapshot(InputArchive ia) throws IOException {
initialized = true;
}
+ /**
+ * Deserialize a snapshot that contains FileHeader from an input archive. It is used by
+ * the admin restore command.
+ *
+ * @param ia the input archive to deserialize from
+ * @param is the CheckInputStream to check integrity
+ *
+ * @throws IOException
+ */
+ public synchronized void deserializeSnapshot(final InputArchive ia, final CheckedInputStream is) throws IOException {
+ // clear the zkDatabase
+ clear();
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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
[GitHub] [zookeeper] tisonkun commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1400501407
> I have refactored the code with some type hierarchy. Would you mind taking a quick look at it?
Better than before and good to go :)
--
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
[GitHub] [zookeeper] li4wang commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1399936945
> I'm thinking of avoiding passing inputStream=null here and there but refactoring with some better type hierarchy. But it can be a follow-up since they're internal interfaces.
Thanks for your feedback @tisonkun. I have refactored the code with some type hierarchy.
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1065877257
##########
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:
Moreover, request throttler does the throttling mechanism by blocking this thread.
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1066055084
##########
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:
Yeah, be aware that you have to do `notifyAll()` after changing the state back to RUNNING to notify the waiting threads. I'll add another comment at the right place.
Additionally, I don't get this `check-wait(timeout)-check-wait(timeout)-...` logic. It doesn't make sense to me, there's no additional fail-safe logic inside the loop, why don't just `wait()`?
Instead, it takes back the lock in every second probably without any benefit. I don't say we have to change that, I just don't understand.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1067627020
##########
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 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?`
Blocking the thread doesn't change the behavior as the request fails before being sent to the server. In addition, as pointed out, it can potentially increase latency.
Since the restore feature is really designed for recovering from rarely happed disaster failure (i.e. quorum lost), I think not accepting any client requests in that scenario would be ok. Thought?
--
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
[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1047914987
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -620,6 +623,48 @@ public void deserializeSnapshot(InputArchive ia) throws IOException {
initialized = true;
}
+ /**
+ * Deserialize a snapshot that contains FileHeader from an input archive. It is used by
+ * the admin restore command.
+ *
+ * @param ia the input archive to deserialize from
+ * @param is the CheckInputStream to check integrity
+ *
+ * @throws IOException
+ */
+ public synchronized void deserializeSnapshot(final InputArchive ia, final CheckedInputStream is) throws IOException {
+ // clear the zkDatabase
+ clear();
Review Comment:
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `ZKDatabase.deserializeSnapshot(...)` indirectly reads with synchronization from `this.dataTree`. Potentially races with unsynchronized write in method `ZKDatabase.clear()`.
Reporting because this access may occur on a background thread.
---
<details><summary><b>ℹ️ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=360842486&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=360842486&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=360842486&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=360842486&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=360842486&lift_comment_rating=5) ]
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1072964684
##########
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:
How about using `volatile` instead of `AtomicReference` to keep it simple and lightweight? In this case, we only need to take care of visibility issue. Only one thread writes the value, so no need for the additional power (i.e. atomicity/CAS) provided by AtomicReference.
With `volatile`, we can ensure once the restore thread creates a new instance, the instance value will be immediately visible to threads that check if it's null.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1083734365
##########
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:
updated
##########
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:
updated
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1065872646
##########
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:
What are you thinking of exactly?
We already block the thread when RequestThrottler or first processor is not yet initialized with the following logic:
```java
while (state == State.INITIAL) {
wait(1000);
}
```
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1067672573
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java:
##########
@@ -583,6 +590,58 @@ public synchronized File takeSnapshot(boolean syncSnap, boolean isSevere, boolea
return snapFile;
}
+ /**
+ * Restores database from a snapshot. It is used by the restore admin server command.
+ *
+ * @param inputStream input stream of snapshot
+ * @Return last processed zxid
+ * @throws IOException
+ */
+ public synchronized long restoreFromSnapshot(final InputStream inputStream) throws IOException {
+ if (inputStream == null) {
+ throw new IllegalArgumentException("InputStream can not be null when restoring from snapshot");
+ }
+
+ long start = Time.currentElapsedTime();
+ LOG.info("Before restore database. lastProcessedZxid={}, nodeCount={}，sessionCount={}",
+ getZKDatabase().getDataTreeLastProcessedZxid(),
+ getZKDatabase().dataTree.getNodeCount(),
+ getZKDatabase().getSessionCount());
+
+ // restore to a new zkDatabase
+ final ZKDatabase newZKDatabase = new ZKDatabase(this.txnLogFactory);
+ final CheckedInputStream cis = new CheckedInputStream(new BufferedInputStream(inputStream), new Adler32());
+ final InputArchive ia = BinaryInputArchive.getArchive(cis);
+ newZKDatabase.deserializeSnapshot(ia, cis);
+ LOG.info("Restored to a new database. lastProcessedZxid={}, nodeCount={}, sessionCount={}",
+ newZKDatabase.getDataTreeLastProcessedZxid(),
+ newZKDatabase.dataTree.getNodeCount(),
+ newZKDatabase.getSessionCount());
+
+ // set the state to MAINTENANCE to stop taking incoming requests
+ setState(State.MAINTENANCE);
Review Comment:
yes, agree.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1067627020
##########
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 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?`
Blocking the thread doesn't change the behavior as the request fails before being sent to the server. In addition, as pointed out, it can potentially increase the latency.
Since the restore feature is really designed for recovering from rarely happed disaster failure (i.e. quorum lost), Not accepting any client requests in that scenario should be ok, right? WDYT?
--
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
[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1065879455
##########
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:
you are right @anmolnar
thanks for your clarification.
we could have a similar loop here
```
while (state == State.MAINTENANCE) {
wait(1000);
}
```
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1067627020
##########
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 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?`
Blocking the thread doesn't change the behavior as the request fails before being sent to the server. In addition, as pointed out, it can potentially increase latency.
Since the restore feature is really designed for recovering from large disaster failure (i.e. quorum lost), I would think not accepting any client requests during maintenance in that scenario is kind of acceptable. Thought?
--
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
[GitHub] [zookeeper] eolivelli merged pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli merged PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1066056379
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java:
##########
@@ -583,6 +590,58 @@ public synchronized File takeSnapshot(boolean syncSnap, boolean isSevere, boolea
return snapFile;
}
+ /**
+ * Restores database from a snapshot. It is used by the restore admin server command.
+ *
+ * @param inputStream input stream of snapshot
+ * @Return last processed zxid
+ * @throws IOException
+ */
+ public synchronized long restoreFromSnapshot(final InputStream inputStream) throws IOException {
+ if (inputStream == null) {
+ throw new IllegalArgumentException("InputStream can not be null when restoring from snapshot");
+ }
+
+ long start = Time.currentElapsedTime();
+ LOG.info("Before restore database. lastProcessedZxid={}, nodeCount={}，sessionCount={}",
+ getZKDatabase().getDataTreeLastProcessedZxid(),
+ getZKDatabase().dataTree.getNodeCount(),
+ getZKDatabase().getSessionCount());
+
+ // restore to a new zkDatabase
+ final ZKDatabase newZKDatabase = new ZKDatabase(this.txnLogFactory);
+ final CheckedInputStream cis = new CheckedInputStream(new BufferedInputStream(inputStream), new Adler32());
+ final InputArchive ia = BinaryInputArchive.getArchive(cis);
+ newZKDatabase.deserializeSnapshot(ia, cis);
+ LOG.info("Restored to a new database. lastProcessedZxid={}, nodeCount={}, sessionCount={}",
+ newZKDatabase.getDataTreeLastProcessedZxid(),
+ newZKDatabase.dataTree.getNodeCount(),
+ newZKDatabase.getSessionCount());
+
+ // set the state to MAINTENANCE to stop taking incoming requests
+ setState(State.MAINTENANCE);
+
+ // set to the new zkDatabase
+ setZKDatabase(newZKDatabase);
+
+ // re-create SessionTrack
+ createSessionTracker();
+
+ LOG.info("After restore database. lastProcessedZxid={}, nodeCount={}, sessionCount={}",
+ getZKDatabase().getDataTreeLastProcessedZxid(),
+ getZKDatabase().dataTree.getNodeCount(),
+ getZKDatabase().getSessionCount());
+
+ // set the state back to RUNNING
+ setState(State.RUNNING);
Review Comment:
After this you have to `notifyAll()`.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1066024566
##########
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:
Thanks for the inputs. I will look into this.
--
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
[GitHub] [zookeeper] li4wang commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
li4wang commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1067627020
##########
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 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?`
Blocking the thread doesn't change the behavior as the request fails before being sent to the server. In addition, as pointed out, it can potentially increase latency.
Since the restore feature is really designed for recovering from large disaster failure (i.e. quorum lost), I would think not accepting any client requests during maintenance in that scenario is kind of acceptable. Thought?
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1068106405
##########
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:
Got you. There're 2 things that crossed my mind to resolve this:
1. Add MAINTENANCE as a running state to `ZooKeeperServer.isRunning()` method.
2. Do not alter the state of ZooKeeperServer, but use a separate volatile variable in this class to block requests temporarly.
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1073992180
##########
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:
Sounds good to me.
--
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
[GitHub] [zookeeper] anmolnar commented on a diff in pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#discussion_r1072282790
##########
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:
+1 for the logging suggestion of @eolivelli .
I also like the Latch approach @li4wang . I think you have to use `AtomicReference<CountDownLatch>` for that, because you create a new instance every time ZK enters into Maintenance mode. In `submitRequest()` you'll try grabbing the reference by `get()` and if it's not null, await for it.
##########
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:
+1 for the logging suggestion of @eolivelli .
I also like the Latch approach @li4wang . I think you have to use `AtomicReference<CountDownLatch>` for that, because you create a new instance every time ZK enters into Maintenance mode. In `submitRequest()` you'll try grabbing the reference by `get()` and if it's not null, await for it.
--
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
[GitHub] [zookeeper] li4wang commented on pull request #1961: ZOOKEEPER-4571: Admin server API for restore database from a snapshot
Posted by "li4wang (via GitHub)" <gi...@apache.org>.
li4wang commented on PR #1961:
URL: https://github.com/apache/zookeeper/pull/1961#issuecomment-1402436211
> @li4wang CI is failing
>
> ```
> [WARNING] Files with unapproved licenses:
> /home/runner/work/zookeeper/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/ZookeeperServerRestoreTest.java
> ```
@eolivelli Sorry, missed this one again. Fixed. Thanks.
--
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