[GitHub] [zookeeper] li4wang commented on pull request #1972: [ZOOKEEPER-4661] Upgrade Jackson Databind to 2.13.4.2 for CVE-2022-42003 CVE-2022-42004

2023-01-20 Thread GitBox


li4wang commented on PR #1972:
URL: https://github.com/apache/zookeeper/pull/1972#issuecomment-1398694133

   LGTM


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli merged pull request #1970: [ZOOKEEPER-4659] Upgrade commons-cli to 1.5.0

2023-01-20 Thread GitBox


eolivelli merged PR #1970:
URL: https://github.com/apache/zookeeper/pull/1970


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1970: [ZOOKEEPER-4659] Upgrade commons-cli to 1.5.0

2023-01-20 Thread GitBox


eolivelli commented on PR #1970:
URL: https://github.com/apache/zookeeper/pull/1970#issuecomment-1398063665

   @cnauroth conflicts resolved, thank you


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] cconroy commented on pull request #1948: Fix add version for container and TTL nodes

2023-01-19 Thread GitBox


cconroy commented on PR #1948:
URL: https://github.com/apache/zookeeper/pull/1948#issuecomment-1397397638

   @maoling I do not have write access so will need someone who does to merge 
this on my behalf. 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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli closed pull request #1957: ZOOKEEPER-4644: update dependencies before release 3.6.4

2023-01-19 Thread GitBox


eolivelli closed pull request #1957: ZOOKEEPER-4644: update dependencies before 
release 3.6.4
URL: https://github.com/apache/zookeeper/pull/1957


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1963: ZOOKEEPER-4649: Upgrade netty to 4.1.86 because of CVE-2022-41915

2023-01-19 Thread GitBox


eolivelli commented on PR #1963:
URL: https://github.com/apache/zookeeper/pull/1963#issuecomment-1396789580

   committed to master, branch-3.8 and branch-3.7


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli merged pull request #1963: ZOOKEEPER-4649: Upgrade netty to 4.1.86 because of CVE-2022-41915

2023-01-19 Thread GitBox


eolivelli merged PR #1963:
URL: https://github.com/apache/zookeeper/pull/1963


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli merged pull request #1964: ZOOKEEPER-4649: Upgrade netty to 4.1.86 because of CVE-2022-41915

2023-01-19 Thread GitBox


eolivelli merged PR #1964:
URL: https://github.com/apache/zookeeper/pull/1964


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1970: [ZOOKEEPER-4659] Upgrade commons-cli to 1.5.0

2023-01-19 Thread GitBox


eolivelli commented on PR #1970:
URL: https://github.com/apache/zookeeper/pull/1970#issuecomment-1396779138

   I have tested the CLI manually and it still works


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] cnauroth commented on a diff in pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress

2023-01-18 Thread GitBox


cnauroth commented on code in PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#discussion_r1074016629


##
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##
@@ -148,9 +156,6 @@ public void 
testServerHostnameVerificationWithHostnameVerificationDisabled() thr
 X509Certificate[] certificateChain = 
createSelfSignedCertifcateChain(IP_ADDRESS, HOSTNAME);
 zkTrustManager.checkServerTrusted(certificateChain, null, mockSocket);
 
-verify(mockInetAddress, times(0)).getHostAddress();

Review Comment:
   Here is another potential idea. I believe the intent of these tests is not 
necessarily to confirm that we called `InetAddress#getHostAddress()` or 
`getHostName()`, but rather to confirm that we performed hostname verification 
in the right circumstances. Therefore, we could try a refactoring as follows:
   
   1. The `ZKTrustManager` constructor currently calls `new 
ZKHostnameVerifier()` directly. Change it to use dependency injection, with the 
constructor receiving an instance of the `HostnameVerifier` interface.
   1. Alternative: if this ends up being too large of a change, keep the 
existing constructor and add a new one annotated `VisibleForTesting` that 
accepts the `HostnameVerifier`.
   2. Keep the existing tests using `ZKHostnameVerifier` so that we're still 
fully covering that code path, using the new Burning Wave setup, even though we 
can't do complete assertions on it.
   3. Remove all existing checks on `InetAddress#getHostAddress()` and 
`getHostName()`, just like you've already done.
   4. Add a few new similar tests that mock and verify the `HostnameVerifier` 
instead of `InetAddress`. We should be able to keep mocking `HostnameVerifier` 
across all JDK versions.
   
   @eolivelli , let me know your thoughts. Thanks!



##
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##
@@ -85,20 +106,7 @@ public static void removeBouncyCastleProvider() throws 
Exception {
 public void setup() throws Exception {
 mockX509ExtendedTrustManager = mock(X509ExtendedTrustManager.class);
 
-mockInetAddress = mock(InetAddress.class);

Review Comment:
   We are still left with a `mockInetAddress` member variable, which could 
cause some confusion since it's not really a mock anymore.
   
   Can the member variable be removed completely? (Maybe not. Maybe it's still 
needed to return from the mock socket.)
   
   If we still need it, then I suggest renaming it to `inetAddress` so that 
it's clearer that it's not really a mock.



-- 
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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-18 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on a diff in pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress

2023-01-18 Thread GitBox


eolivelli commented on code in PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#discussion_r1073546869


##
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##
@@ -148,9 +156,6 @@ public void 
testServerHostnameVerificationWithHostnameVerificationDisabled() thr
 X509Certificate[] certificateChain = 
createSelfSignedCertifcateChain(IP_ADDRESS, HOSTNAME);
 zkTrustManager.checkServerTrusted(certificateChain, null, mockSocket);
 
-verify(mockInetAddress, times(0)).getHostAddress();

Review Comment:
   I knew that we are going to lose these assertions, the problem is that the 
tests verified that these two methods were not called (or called N times).
   We cannot spy the InetAddress object anymore.
   
   we could create two "versions" of this test, if we are on JDK19+ we do it 
this way, and if we are on older JDKs we keep the mock and run the test the 
same way as before.
   I don't think that there will be short term plans to abandon older JDKs soon 
(years)
   
   



-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1968: Cleanup code in the DataTree class

2023-01-18 Thread GitBox


eolivelli commented on PR #1968:
URL: https://github.com/apache/zookeeper/pull/1968#issuecomment-1387083728

   @tisonkun can you please crate a JIRA and change the title and the commit 
message ?


-- 
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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-17 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] cnauroth commented on a diff in pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress

2023-01-17 Thread GitBox


cnauroth commented on code in PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#discussion_r1072845647


##
zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java:
##
@@ -148,9 +156,6 @@ public void 
testServerHostnameVerificationWithHostnameVerificationDisabled() thr
 X509Certificate[] certificateChain = 
createSelfSignedCertifcateChain(IP_ADDRESS, HOSTNAME);
 zkTrustManager.checkServerTrusted(certificateChain, null, mockSocket);
 
-verify(mockInetAddress, times(0)).getHostAddress();

Review Comment:
   These `verify` calls are used to assert for expected behavior of hostname 
verification in various use cases. Without the `verify` calls, we would lose 
some test coverage, and I think several of these tests would all be testing the 
same thing.
   
   I'm not familiar with the new burningwave library. Do you know if there are 
any options it offers for intercepting these calls so that we could try to 
preserve this test coverage?



-- 
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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-17 Thread GitBox


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` 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` 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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] chufe-dremio commented on pull request #1956: ZOOKEEPER-4644: update dependencies before release 3.6.4

2023-01-17 Thread GitBox


chufe-dremio commented on PR #1956:
URL: https://github.com/apache/zookeeper/pull/1956#issuecomment-1385117776

   Did this merge also go to Zookeeper 3.8?
   Somehow we see the CVEs reported:  CVE-2022-42003(7.5), CVE-2022-42004(7.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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1959: [ZOOKEEPER-4647] Tests don't pass on JDK20 because we try to mock InetAddress

2023-01-17 Thread GitBox


eolivelli commented on PR #1959:
URL: https://github.com/apache/zookeeper/pull/1959#issuecomment-1385045432

   @li4wang @tisonkun @anmolnar @symat @cnauroth PTAL


-- 
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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-16 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-13 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-13 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-13 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-13 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-13 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-13 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-13 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] dongjoon-hyun commented on pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-12 Thread GitBox


dongjoon-hyun commented on PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#issuecomment-1381020406

   Thank you so much!


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] cnauroth commented on pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-12 Thread GitBox


cnauroth commented on PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#issuecomment-1381003653

   I have committed this to master, branch-3.8 and branch-3.7. @dongjoon-hyun , 
thank you for the patch. @VinodAnandan , thank you for code reviewing.


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] cnauroth merged pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-12 Thread GitBox


cnauroth merged PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969


-- 
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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-12 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1688: ZOOKEEPER-4287: Upgrade prometheus client library version to 0.10.0

2023-01-12 Thread GitBox


eolivelli commented on PR #1688:
URL: https://github.com/apache/zookeeper/pull/1688#issuecomment-1380138258

   I think that we can include this change in 3.9.0


-- 
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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-12 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1940: ZOOKEEPER-4628: Upgrade jackson-databind dependency for CVE-2022-4200…

2023-01-12 Thread GitBox


eolivelli commented on PR #1940:
URL: https://github.com/apache/zookeeper/pull/1940#issuecomment-1380122551

   @Ghatage sorry for late reply.
   could you please fix the conflicts ?
   
   also could you also take this change in order to add here the license files 
for Jackson ?
   
https://github.com/apache/zookeeper/tree/master/zookeeper-server/src/main/resources/lib


-- 
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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-11 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-11 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-11 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-11 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-11 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-11 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] dongjoon-hyun commented on a diff in pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-10 Thread GitBox


dongjoon-hyun commented on code in PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#discussion_r1066529592


##
pom.xml:
##
@@ -918,6 +918,11 @@
   maven-bundle-plugin
   5.1.1
 
+
+  org.cyclonedx
+  cyclonedx-maven-plugin
+  2.7.3

Review Comment:
   Thank you so much, @VinodAnandan !



-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] VinodAnandan commented on a diff in pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-10 Thread GitBox


VinodAnandan commented on code in PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#discussion_r1066512828


##
pom.xml:
##
@@ -918,6 +918,11 @@
   maven-bundle-plugin
   5.1.1
 
+
+  org.cyclonedx
+  cyclonedx-maven-plugin
+  2.7.3

Review Comment:
   @dongjoon-hyun sure, I will try 2.7.4 update in a separate PR after this PR.



-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] dongjoon-hyun commented on pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-10 Thread GitBox


dongjoon-hyun commented on PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#issuecomment-1378115918

   Thank you for review, @VinodAnandan and @cnauroth !


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] dongjoon-hyun commented on pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-10 Thread GitBox


dongjoon-hyun commented on PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#issuecomment-1378115702

   Hi, @cnauroth . I want to keep this PR AS-IS with 2.7.3 because it's the 
verified version in various environments and cases. 
   
   BTW, the CI failure looks irrelevant to this PR.
   ```
   [ERROR] Errors: 
   [ERROR]   ReadOnlyModeTest.testConnectionEvents:202 » Timeout Failed to 
connect in read-...
   [INFO] 
   [ERROR] Tests run: 2999, Failures: 0, Errors: 1, Skipped: 4
   ```


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] dongjoon-hyun commented on a diff in pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-10 Thread GitBox


dongjoon-hyun commented on code in PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#discussion_r1066489850


##
pom.xml:
##
@@ -918,6 +918,11 @@
   maven-bundle-plugin
   5.1.1
 
+
+  org.cyclonedx
+  cyclonedx-maven-plugin
+  2.7.3

Review Comment:
   Actually, I found it doesn't work in some cases, @VinodAnandan . While 
helping other projects, I tried and reverted to back 2.7.3.
   - https://cwiki.apache.org/confluence/display/COMDEV/SBOM
   
   Could you try that in a separate PR after this PR?



-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] VinodAnandan commented on a diff in pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-10 Thread GitBox


VinodAnandan commented on code in PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#discussion_r1066463919


##
pom.xml:
##
@@ -918,6 +918,11 @@
   maven-bundle-plugin
   5.1.1
 
+
+  org.cyclonedx
+  cyclonedx-maven-plugin
+  2.7.3

Review Comment:
   The cyclonedx-maven-plugin-2.7.4 was released last week, more information - 
https://github.com/CycloneDX/cyclonedx-maven-plugin/releases/tag/cyclonedx-maven-plugin-2.7.4



-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] VinodAnandan commented on a diff in pull request #1969: ZOOKEEPER-4657: Publish SBOM artifacts

2023-01-10 Thread GitBox


VinodAnandan commented on code in PR #1969:
URL: https://github.com/apache/zookeeper/pull/1969#discussion_r1066462607


##
pom.xml:
##
@@ -918,6 +918,11 @@
   maven-bundle-plugin
   5.1.1
 
+
+  org.cyclonedx
+  cyclonedx-maven-plugin
+  2.7.3

Review Comment:
   ```suggestion
 2.7.4
   ```



-- 
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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



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

2023-01-10 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



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

2023-01-09 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-09 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: ZOOKEEPER-4655: Communicate the Zxid that triggered a WatchEvent to fire

2023-01-09 Thread GitBox


PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1376113488

   @eolivelli Done


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] li4wang commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2023-01-09 Thread GitBox


li4wang commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1376105969

   Thanks @eolivelli for reviewing it.
   
   Can someone else please also take a look at it? 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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-09 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2023-01-09 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] ScottBrenner commented on pull request #1938: Bump actions/checkout to v3 in GitHub Actions workflows

2023-01-06 Thread GitBox


ScottBrenner commented on PR #1938:
URL: https://github.com/apache/zookeeper/pull/1938#issuecomment-1374223369

   Could the failing checks be retried?


-- 
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-unsubscr...@zookeeper.apache.org

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



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

2023-01-06 Thread GitBox


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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] PapaCharlie commented on pull request #1950: Communicate the Zxid that triggered a WatchEvent to fire

2023-01-05 Thread GitBox


PapaCharlie commented on PR #1950:
URL: https://github.com/apache/zookeeper/pull/1950#issuecomment-1372345411

   Hey @eolivelli, thanks for taking a look at this! Happy to hear this looks 
good. I'm just lucky there was room in the existing API for me to squeeze this.
   
   Let me update the commit message and poke around the docs and see where I 
can make it more obvious that this is available.
   
   Regarding the new client/old server issue: yeah old servers will always send 
-1, so it's a pretty clear way to know whether you're talking to an old server. 
I'll make sure to state that in the docs.
   
   Let me take a look at curator and see what can be done. It may be feasible 
to expose this in a sane way


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] wg1026688210 commented on pull request #262: ZOOKEEPER-2789: Reassign `ZXID` for solving 32bit overflow problem

2023-01-04 Thread GitBox


wg1026688210 commented on PR #262:
URL: https://github.com/apache/zookeeper/pull/262#issuecomment-1371831863

   Are there other conditions that would cause epcho to increase except the 
counter bit rolling over ?  If not ,no matter how we adjust the bit of epcho, 
will the number of changes of zxid not exceed 9,223,372,036,854,775,807 ?  If 
existing and the total bit of zxid is 64, shall we add a config to update the 
counter bit .


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] wg1026688210 commented on pull request #262: ZOOKEEPER-2789: Reassign `ZXID` for solving 32bit overflow problem

2023-01-04 Thread GitBox


wg1026688210 commented on PR #262:
URL: https://github.com/apache/zookeeper/pull/262#issuecomment-1371825714

   @asdf2014 @phunt  Is there a solution  in  the  rolling over of counter bit  
now ,  we have this problem at version 3.4.13


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] li4wang commented on pull request #1943: ZOOKEEPER-4570: Admin server API for taking snapshot and stream out data

2023-01-04 Thread GitBox


li4wang commented on PR #1943:
URL: https://github.com/apache/zookeeper/pull/1943#issuecomment-1371249655

   Great, thanks @eolivelli


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli merged pull request #1943: ZOOKEEPER-4570: Admin server API for taking snapshot and stream out data

2023-01-04 Thread GitBox


eolivelli merged PR #1943:
URL: https://github.com/apache/zookeeper/pull/1943


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1947: ZOOKEEPER-4656: Allow ZooKeeperAdmin creation with custom HostProvider

2023-01-04 Thread GitBox


eolivelli commented on PR #1947:
URL: https://github.com/apache/zookeeper/pull/1947#issuecomment-1370997242

   I would prefer to not commit the new constructor and then remove it.
   There is no hurry in committing a patch, and sometimes if someone adds a new 
API it may stay there forever if we forget about it.
   So overall I believe that it is better to add the builder and do not add a 
new public constructor..
   
   For the test, we can a very simple test that bootstraps an instance using 
the new builder.
   
   
   


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] jowiho commented on pull request #1947: ZOOKEEPER-4656: Allow ZooKeeperAdmin creation with custom HostProvider

2023-01-04 Thread GitBox


jowiho commented on PR #1947:
URL: https://github.com/apache/zookeeper/pull/1947#issuecomment-1370934346

   Thanks for your feedback @eolivelli, and for creating the JIRA issue. I've 
added the JIRA ID to the PR title and commit message.
   
   > can you please add a test ?
   
   I've considered it, but I couldn't come up with a good way to test it. There 
currently are no other tests for `ZookeeperAdmin`. Even if I'd add a new test 
class, I'd still struggle to add a meaningful test for the constructor overload 
that I'm adding in this PR. Basically the test would need to verify that the 
constructor propagates the `hostProvider` parameter to its `ZooKeeper` super 
class. But AFAICT there is no way to access the `hostProvider` in `ZooKeeper` 
directly, in order to test if it's the same as the parameter that I passed to 
`ZookeeperAdmin`. Do you have a clever idea on how to test this without an 
overly complex test setup?
   
   > Add a new overloaded constructor will add some tech debt. Would you please 
think about adding a Builder API for ZooKeeperAdmin ? this way in the future it 
will be simpler to add more customisation points
   
   I agree that a Builder API would be a very useful addition. But perhaps, in 
order to keep the PR small, that would better be done in a separate PR?


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1943: ZOOKEEPER-4570: Admin server API for taking snapshot and stream out data

2023-01-04 Thread GitBox


eolivelli commented on PR #1943:
URL: https://github.com/apache/zookeeper/pull/1943#issuecomment-1370681894

   @li4wang I have merged master branch into you branch in order to fix CI


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1943: ZOOKEEPER-4570: Admin server API for taking snapshot and stream out data

2023-01-04 Thread GitBox


eolivelli commented on PR #1943:
URL: https://github.com/apache/zookeeper/pull/1943#issuecomment-1370663660

   I will commit this patch as soon as CI passes.
   I have triggered a new run


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] jonmv commented on pull request #1925: ZOOKEEPER-4541 Ephemeral znode owned by closed session visible in 1 of 3 servers

2023-01-03 Thread GitBox


jonmv commented on PR #1925:
URL: https://github.com/apache/zookeeper/pull/1925#issuecomment-1369710624

   Any further thoughts on this @breed, @eolivelli ? 


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1968: Cleanup code in the DataTree class

2022-12-28 Thread GitBox


sonatype-lift[bot] commented on code in PR #1968:
URL: https://github.com/apache/zookeeper/pull/1968#discussion_r1058190304


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java:
##
@@ -781,7 +773,7 @@ public List getACL(String path, Stat stat) throws 
KeeperException.NoNodeExc
 if (stat != null) {
 n.copyStat(stat);
 }
-return new ArrayList(aclCache.convertLong(n.acl));
+return new ArrayList<>(aclCache.convertLong(n.acl));

Review Comment:
   https://lift.sonatype.com/api/commentimage/fixrate/20/display.svg;>
   
    5 similar findings have been found in this PR
   
   ---
   
   *THREAD_SAFETY_VIOLATION:*  Read/Write race. Non-private method 
`DataTree.getACL(...)` indirectly reads with synchronization from container 
`this.aclCache.longKeyMap` via call to `Map.get(...)`. Potentially races with 
unsynchronized write in method `DataTree.deserialize(...)`.
Reporting because this access may occur on a background thread.
   
   ---
   
    Expand here to view all instances of this 
finding
 
 
   
   
   
   | **File Path** | **Line Number** |
   | - | - |
   | zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java | 
[1467](https://github.com/apache/zookeeper/blob/f2bc11c557676e8983bd501c0eb03a67da30fa92/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L1467)
 |
   | zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java | 
[195](https://github.com/apache/zookeeper/blob/f2bc11c557676e8983bd501c0eb03a67da30fa92/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L195)
 |
   | zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java | 
[1368](https://github.com/apache/zookeeper/blob/f2bc11c557676e8983bd501c0eb03a67da30fa92/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L1368)
 |
   | zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java | 
[1440](https://github.com/apache/zookeeper/blob/f2bc11c557676e8983bd501c0eb03a67da30fa92/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L1440)
 |
   | zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java | 
[461](https://github.com/apache/zookeeper/blob/f2bc11c557676e8983bd501c0eb03a67da30fa92/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L461)
 |
   https://lift.sonatype.com/results/github.com/apache/zookeeper/01GNBYXJDSMJ3R1WR62NJGA4AZ?t=Infer|THREAD_SAFETY_VIOLATION"
 target="_blank">Visit the Lift Web Console to find more details in your 
report.
   
   
   
   ---
   
   ℹ️ Learn about @sonatype-lift commands
   
   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 ` | 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.
   [Click here](https://github.com/apps/sonatype-lift/installations/new) 
to add LiftBot to another repo.
   
   
   
   ---
   
   Was this a good recommendation?
   [ [ Not 
relevant](https://www.sonatype.com/lift-comment-rating?comment=364338927_comment_rating=1)
 ] - [ [ Won't 
fix](https://www.sonatype.com/lift-comment-rating?comment=364338927_comment_rating=2)
 ] - [ [ Not critical, will 
fix](https://www.sonatype.com/lift-comment-rating?comment=364338927_comment_rating=3)
 ] - [ [ Critical, will 
fix](https://www.sonatype.com/lift-comment-rating?comment=364338927_comment_rating=4)
 ] - [ [ Critical, fixing 
now](https://www.sonatype.com/lift-comment-rating?comment=364338927_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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] tisonkun opened a new pull request, #1968: Cleanup code in the DataTree class

2022-12-28 Thread GitBox


tisonkun opened a new pull request, #1968:
URL: https://github.com/apache/zookeeper/pull/1968

   Today I read the `DataTree` class and do these cleanups along with the path.
   
   cc @symat @eolivelli 


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] tisonkun commented on pull request #1106: ZOOKEEPER-1416 - Persistent, recursive watchers

2022-12-28 Thread GitBox


tisonkun commented on PR #1106:
URL: https://github.com/apache/zookeeper/pull/1106#issuecomment-1366449561

   @Randgalt I get your point.
   
   > similar to code above this and process the watch if needed
   
   Although, this can be more trick than it first looks like. Because a 
persistent watch is always set, only `node.stat.getMzxid() > relativeZxid` 
indicates a `NodeDataChanged` event. Either the node exists or not cannot 
indicate `NodeCreated` or `NodeDeleted`.
   
   But I think I know what we can do and your original thought now :)


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] Randgalt commented on pull request #1106: ZOOKEEPER-1416 - Persistent, recursive watchers

2022-12-27 Thread GitBox


Randgalt commented on PR #1106:
URL: https://github.com/apache/zookeeper/pull/1106#issuecomment-1366445670

   > > Yes. It’s the triggering of missed events that I’m concerned about. It 
would be easy to add but I’m unsure if it’s needed.
   > 
   > @Randgalt I run into this case today. How can it be easy to add? At least 
we need to associate zxid with the request and the data tree node, as well as 
bookkeep the changes. Then we can filter the changes between the client 
setwatches request and the server state. Or we send some duplicate but the 
client can filter with some revisions.
   
   The persistentWatches should be easy. Something like:
   
   ```
   for (String path : persistentWatches) {
   DataNode node = getNode(path);
   // determine if there's a change similar to code above this and 
process the watch if needed
   this.childWatches.addWatch(path, watcher, 
WatcherMode.PERSISTENT);
   this.dataWatches.addWatch(path, watcher, WatcherMode.PERSISTENT);
   }
   ```
   
   persistentRecursiveWatches are harder. For each persistent recursive watch 
you'd need to walk down child paths of each watch, get the node and determine 
if the watch needs to be called. Depending on the level of the watch it could 
end up walking the entire database but there's no choice.


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] tisonkun commented on pull request #1106: ZOOKEEPER-1416 - Persistent, recursive watchers

2022-12-27 Thread GitBox


tisonkun commented on PR #1106:
URL: https://github.com/apache/zookeeper/pull/1106#issuecomment-1366400574

   > Yes. It’s the triggering of missed events that I’m concerned about. It 
would be easy to add but I’m unsure if it’s needed.
   
   @Randgalt I run into this case today. How can it be easy to add? At least we 
need to associate zxid with the request and the data tree node, as well as 
bookkeep the changes. Then we can filter the changes between the client 
setwatches request and the server state. Or we send some duplicate but the 
client can filter with some revisions.


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] brahmareddybattula commented on pull request #1953: ZOOKEEPER-4554 : upgrade netty 4.1.76.Final to 4.1.77.Final

2022-12-26 Thread GitBox


brahmareddybattula commented on PR #1953:
URL: https://github.com/apache/zookeeper/pull/1953#issuecomment-1365322235

   lgtm. (non-binding)


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-21 Thread GitBox


li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1055081235


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##
@@ -257,17 +263,84 @@ protected void doGet(
 for (Map.Entry entry : parameterMap.entrySet()) {
 kwargs.put(entry.getKey(), entry.getValue()[0]);
 }
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
 
 // Run the command
-CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, 
kwargs);
+final CommandResponse cmdResponse = Commands.runCommand(cmd, 
zkServer, kwargs, null, authInfo, request);
+response.setStatus(cmdResponse.getStatusCode());
 
-// Format and print the output of the command
-CommandOutputter outputter = new JsonOutputter();
-response.setStatus(HttpServletResponse.SC_OK);
+final Map headers = cmdResponse.getHeaders();
+for (final Map.Entry 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) {
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
+final CommandResponse cmdResponse = 
Commands.runCommand(cmdName, zkServer, null, request.getInputStream(), 
authInfo, request);

Review Comment:
   @sonatype-lift ignore
   
   Although http header is user-supplied data, but we don't call out to OS 
commands in the code base, so it can be ignored.
   
   ProviderRegistry.addOrUpdateProvider(...)  is an existing API before this PR.




-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-21 Thread GitBox


li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054858542


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/Commands.java:
##
@@ -92,24 +112,97 @@ public static void registerCommand(Command command) {
  *
  * @param cmdName
  * @param zkServer
- * @param kwargs String-valued keyword arguments to the command
+ * @param kwargs String-valued keyword arguments to the command from HTTP 
GET request
  *(may be null if command requires no additional arguments)
+ * @param inputStream inputStream from HTTP POST request
+ *(null if it's HTTP GET request)
+ * @param authInfo auth info for auth check
+ *(null if command requires no auth check)
+ * @param request HTTP request
  * @return Map representing response to command containing at minimum:
  *- "command" key containing the command's primary name
  *- "error" key containing a String error message or null if no error
  */
 public static CommandResponse runCommand(
-String cmdName,
-ZooKeeperServer zkServer,
-Map kwargs) {
+String cmdName,
+ZooKeeperServer zkServer,
+Map kwargs,
+InputStream inputStream,
+String authInfo,
+HttpServletRequest request) {
 Command command = getCommand(cmdName);
 if (command == null) {
-return new CommandResponse(cmdName, "Unknown command: " + cmdName);
+// set the status code to 200 to keep the current behavior of 
existing commands
+LOG.warn("Unknown command: {}", cmdName);
+return new CommandResponse(cmdName, "Unknown command: " + cmdName, 
HttpServletResponse.SC_OK);
 }
 if (command.isServerRequired() && (zkServer == null || 
!zkServer.isRunning())) {
-return new CommandResponse(cmdName, "This ZooKeeper instance is 
not currently serving requests");
+// set the status code to 200 to keep the current behavior of 
existing commands
+LOG.warn("This ZooKeeper instance is not currently serving 
requests for command: {}", cmdName);
+return new CommandResponse(cmdName, "This ZooKeeper instance is 
not currently serving requests", HttpServletResponse.SC_OK);
 }
-return command.run(zkServer, kwargs);
+
+final AuthRequest authRequest = command.getAuthRequest();
+if (authRequest != null) {
+if (authInfo == null) {
+LOG.warn("Auth info is missing for command: {}", cmdName);
+return new CommandResponse(cmdName, "Auth info is missing for 
the command", HttpServletResponse.SC_UNAUTHORIZED);
+}
+try {
+final List ids = handleAuthentication(request, authInfo);
+handleAuthorization(zkServer, ids, 
authRequest.getPermission(), authRequest.getPath());
+} catch (final KeeperException.AuthFailedException e) {
+return new CommandResponse(cmdName, "Not authenticated", 
HttpServletResponse.SC_UNAUTHORIZED);
+} catch (final KeeperException.NoAuthException e) {
+return new CommandResponse(cmdName, "Not authorized", 
HttpServletResponse.SC_FORBIDDEN);
+} catch (final Exception e) {
+LOG.warn("Error occurred during auth for command: {}", 
cmdName, e);
+return new CommandResponse(cmdName, "Error occurred during 
auth", HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+}
+}
+return command.run(zkServer, kwargs, inputStream);
+}
+
+private static List handleAuthentication(final HttpServletRequest 
request, final String authInfo) throws KeeperException.AuthFailedException {
+final String[] authData = authInfo.split(AUTH_INFO_SEPARATOR);
+// for IP and x509, auth info only contains the schema and Auth Id 
will be extracted from HTTP request
+if (authData.length != 1 && authData.length != 2) {
+LOG.warn("Invalid auth info: {}", authInfo);
+throw new KeeperException.AuthFailedException();
+}
+
+final String schema = authData[0];
+final ServerAuthenticationProvider authProvider = 
ProviderRegistry.getServerProvider(schema);
+if (authProvider != null) {
+try {
+final byte[] auth = authData.length == 2 ? 
authData[1].getBytes(StandardCharsets.UTF_8) : null;
+final List ids = 
authProvider.handleAuthentication(request, auth);
+if (ids.isEmpty()) {
+LOG.warn("Auth Id list is empty: {}", authInfo);

Review Comment:
   fixed



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

[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-21 Thread GitBox


li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054844144


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##
@@ -257,17 +263,84 @@ protected void doGet(
 for (Map.Entry entry : parameterMap.entrySet()) {
 kwargs.put(entry.getKey(), entry.getValue()[0]);
 }
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
 
 // Run the command
-CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, 
kwargs);
+final CommandResponse cmdResponse = Commands.runCommand(cmd, 
zkServer, kwargs, null, authInfo, request);

Review Comment:
   @sonatype-lift ignore
   
   ProviderRegistry.addOrUpdateProvider() is not new code from this PR.



-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-21 Thread GitBox


sonatype-lift[bot] commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054844182


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##
@@ -257,17 +263,84 @@ protected void doGet(
 for (Map.Entry entry : parameterMap.entrySet()) {
 kwargs.put(entry.getKey(), entry.getValue()[0]);
 }
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
 
 // Run the command
-CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, 
kwargs);
+final CommandResponse cmdResponse = Commands.runCommand(cmd, 
zkServer, kwargs, null, authInfo, request);

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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-21 Thread GitBox


li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054844144


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##
@@ -257,17 +263,84 @@ protected void doGet(
 for (Map.Entry entry : parameterMap.entrySet()) {
 kwargs.put(entry.getKey(), entry.getValue()[0]);
 }
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
 
 // Run the command
-CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, 
kwargs);
+final CommandResponse cmdResponse = Commands.runCommand(cmd, 
zkServer, kwargs, null, authInfo, request);

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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-21 Thread GitBox


li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054752855


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##
@@ -620,6 +623,43 @@ 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();

Review Comment:
   @sonatype-lift ignore
   
   This API is called from a thread-safe context, as the  zkDatabase  is a 
local variable in ZookeeperServer.restoreFromSnapshot() method.



-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] li4wang commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-21 Thread GitBox


li4wang commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1054743602


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##
@@ -257,17 +263,84 @@ protected void doGet(
 for (Map.Entry entry : parameterMap.entrySet()) {
 kwargs.put(entry.getKey(), entry.getValue()[0]);
 }
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
 
 // Run the command
-CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, 
kwargs);
+final CommandResponse cmdResponse = Commands.runCommand(cmd, 
zkServer, kwargs, null, authInfo, request);
+response.setStatus(cmdResponse.getStatusCode());
 
-// Format and print the output of the command
-CommandOutputter outputter = new JsonOutputter();
-response.setStatus(HttpServletResponse.SC_OK);
+final Map headers = cmdResponse.getHeaders();
+for (final Map.Entry 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) {
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
+final CommandResponse cmdResponse = 
Commands.runCommand(cmdName, zkServer, null, request.getInputStream(), 
authInfo, request);
+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 
   
   the link string is defined in the zk code base, not user defined data, so 
there should not be vulnerable to XSS.



-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] symat commented on pull request #1967: ZOOKEEPER-4654: Fix C client test compilation error in Util.cc.

2022-12-18 Thread GitBox


symat commented on PR #1967:
URL: https://github.com/apache/zookeeper/pull/1967#issuecomment-1356843435

   I merged the fix to all branches (3.6+). Thank you for the fox @cnauroth !


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] symat commented on pull request #1967: ZOOKEEPER-4654: Fix C client test compilation error in Util.cc.

2022-12-18 Thread GitBox


symat commented on PR #1967:
URL: https://github.com/apache/zookeeper/pull/1967#issuecomment-1356842848

   merging now to all active branches


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] curie71 commented on pull request #1962: ZOOKEEPER-4648 FinalRequestProcessor addAuditLog after the process of request maybe better.

2022-12-17 Thread GitBox


curie71 commented on PR #1962:
URL: https://github.com/apache/zookeeper/pull/1962#issuecomment-1356270372

   @dobozysaurus Could you please to take a look at this issue?
   I found only the request audit log is recorded here in zookeeper.
   But I think "the err code after request processing" or a "response log" is 
also important for audit.


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] asfgit closed pull request #1951: [ZOOKEEPER-4641] GH CI fails with 'error: implicit declaration of function ‘FIPS_mode’ [-Werror=implicit-function-declaration]'

2022-12-15 Thread GitBox


asfgit closed pull request #1951: [ZOOKEEPER-4641] GH CI fails with 'error: 
implicit declaration of function ‘FIPS_mode’ 
[-Werror=implicit-function-declaration]'
URL: https://github.com/apache/zookeeper/pull/1951


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] symat commented on pull request #1951: [ZOOKEEPER-4641] GH CI fails with 'error: implicit declaration of function ‘FIPS_mode’ [-Werror=implicit-function-declaration]'

2022-12-15 Thread GitBox


symat commented on PR #1951:
URL: https://github.com/apache/zookeeper/pull/1951#issuecomment-1352962603

   I agree, the patch is already helps - at least the C client can be compiled 
now :)
   Merging it now


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] eolivelli commented on pull request #1951: [ZOOKEEPER-4641] GH CI fails with 'error: implicit declaration of function ‘FIPS_mode’ [-Werror=implicit-function-declaration]'

2022-12-15 Thread GitBox


eolivelli commented on PR #1951:
URL: https://github.com/apache/zookeeper/pull/1951#issuecomment-1352946039

   @symat I think that we should merge this an then follow up on the flaky test.
   
   @ztzg maybe you could also help here ?


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] sonatype-lift[bot] commented on a diff in pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-14 Thread GitBox


sonatype-lift[bot] commented on code in PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#discussion_r1049123156


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java:
##
@@ -257,17 +263,84 @@ protected void doGet(
 for (Map.Entry entry : parameterMap.entrySet()) {
 kwargs.put(entry.getKey(), entry.getValue()[0]);
 }
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
 
 // Run the command
-CommandResponse cmdResponse = Commands.runCommand(cmd, zkServer, 
kwargs);
+final CommandResponse cmdResponse = Commands.runCommand(cmd, 
zkServer, kwargs, null, authInfo, request);
+response.setStatus(cmdResponse.getStatusCode());
 
-// Format and print the output of the command
-CommandOutputter outputter = new JsonOutputter();
-response.setStatus(HttpServletResponse.SC_OK);
+final Map headers = cmdResponse.getHeaders();
+for (final Map.Entry 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) {
+final String authInfo = 
request.getHeader(HttpHeader.AUTHORIZATION.asString());
+final CommandResponse cmdResponse = 
Commands.runCommand(cmdName, zkServer, null, request.getInputStream(), 
authInfo, request);
+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
   
   ---
   
   ℹ️ Learn about @sonatype-lift commands
   
   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 ` | 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.
   [Click here](https://github.com/apps/sonatype-lift/installations/new) 
to add LiftBot to another repo.
   
   
   
   ---
   
   Was this a good recommendation?
   [ [ Not 

[GitHub] [zookeeper] li4wang commented on pull request #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-14 Thread GitBox


li4wang commented on PR #1966:
URL: https://github.com/apache/zookeeper/pull/1966#issuecomment-1352431449

   This PR was created on top of admin snapshot PR 
https://github.com/apache/zookeeper/pull/1943 and admin restore PR 
https://github.com/apache/zookeeper/pull/1961, as they are currently blocked by 
the CI build pipeline issue.
   
   The patch for this PR is in 
[322458e](https://github.com/apache/zookeeper/pull/1966/commits/322458ec920c23cb7bfe778250e1574565dce464)


-- 
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-unsubscr...@zookeeper.apache.org

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



[GitHub] [zookeeper] li4wang opened a new pull request, #1966: ZOOKEEPER-4639: Provide auth support for admin server APIs

2022-12-14 Thread GitBox


li4wang opened a new pull request, #1966:
URL: https://github.com/apache/zookeeper/pull/1966

   Provide auth support including digest, x509 and IP for admin server commands.
   
   Author: Li Wang [liw...@apple.com](mailto:liw...@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-unsubscr...@zookeeper.apache.org

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



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

2022-12-14 Thread GitBox


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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2022-12-14 Thread GitBox


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 headers = cmdResponse.getHeaders();
+for (final Map.Entry 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-unsubscr...@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
us...@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

2022-12-14 Thread GitBox


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 headers = cmdResponse.getHeaders();
+for (final Map.Entry 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-unsubscr...@zookeeper.apache.org

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



  1   2   3   4   5   6   7   8   9   10   >