[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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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…
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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.
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.
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]'
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]'
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]'
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
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
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
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
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
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
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