[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user asfgit closed the pull request at: https://github.com/apache/zookeeper/pull/545 ---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/545#discussion_r208507660 --- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java --- @@ -122,8 +122,8 @@ final Map outstandingChangesForPath = new HashMap(); -protected ServerCnxnFactory serverCnxnFactory; -protected ServerCnxnFactory secureServerCnxnFactory; +ServerCnxnFactory serverCnxnFactory; --- End diff -- Intellij suggested. I think because it's more restrictive. ---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user hanm commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/545#discussion_r208451942 --- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java --- @@ -122,8 +122,8 @@ final Map outstandingChangesForPath = new HashMap(); -protected ServerCnxnFactory serverCnxnFactory; -protected ServerCnxnFactory secureServerCnxnFactory; +ServerCnxnFactory serverCnxnFactory; --- End diff -- other than this, patch LGTM! ---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user hanm commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/545#discussion_r208451880 --- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java --- @@ -122,8 +122,8 @@ final Map outstandingChangesForPath = new HashMap(); -protected ServerCnxnFactory serverCnxnFactory; -protected ServerCnxnFactory secureServerCnxnFactory; +ServerCnxnFactory serverCnxnFactory; --- End diff -- `protected` is removed here. Is this intended? ---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/545#discussion_r206120103
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -866,6 +866,9 @@ public void setServerCnxnFactory(ServerCnxnFactory
factory) {
}
public ServerCnxnFactory getServerCnxnFactory() {
+if (secureServerCnxnFactory != null) {
+return secureServerCnxnFactory;
+}
return serverCnxnFactory;
}
--- End diff --
Makes sense.
---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/545#discussion_r206012592
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -866,6 +866,9 @@ public void setServerCnxnFactory(ServerCnxnFactory
factory) {
}
public ServerCnxnFactory getServerCnxnFactory() {
+if (secureServerCnxnFactory != null) {
+return secureServerCnxnFactory;
+}
return serverCnxnFactory;
}
--- End diff --
I think alternatively we can kill the `setSecureServerCnxnFactory` and have
something like `
public void setServerCnxnFactory(ServerCnxnFactory factory) {
if (secure) {
secureServerCnxnFactory = factory;
} else {
serverCnxnFactory = factory;
}
}`
The basic idea is to make code base consistent and easier to read. Having
mixed methods just cost more brain power to reason about (albeit not a big one
in this case).
---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/545#discussion_r205729889 --- Diff: src/java/test/org/apache/zookeeper/server/persistence/UtilTest.java --- @@ -0,0 +1,91 @@ +/** --- End diff -- I had to make a bunch of refactorings to move all of these unit tests to the right place. Sorry for polluting the pull request, I can move it to a separate ticket if you want. ---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/545#discussion_r205729651
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -866,6 +866,9 @@ public void setServerCnxnFactory(ServerCnxnFactory
factory) {
}
public ServerCnxnFactory getServerCnxnFactory() {
+if (secureServerCnxnFactory != null) {
+return secureServerCnxnFactory;
+}
return serverCnxnFactory;
}
--- End diff --
I will look into it. Original issue was on the caller side which didn't
have the logic to probe which ServerCnxnFactory is available, so I probably
have to make it more clever.
Though I'm not entirely convinced that it should be the caller's
responsibility to deal with the problem. He just want something that implements
the interface. Why should make this their problem?
---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user breed commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/545#discussion_r205663866 --- Diff: src/java/test/org/apache/zookeeper/server/persistence/UtilTest.java --- @@ -0,0 +1,91 @@ +/** --- End diff -- did you mean for this file to be part of the patch? ---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user breed commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/545#discussion_r205663822
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -866,6 +866,9 @@ public void setServerCnxnFactory(ServerCnxnFactory
factory) {
}
public ServerCnxnFactory getServerCnxnFactory() {
+if (secureServerCnxnFactory != null) {
+return secureServerCnxnFactory;
+}
return serverCnxnFactory;
}
--- End diff --
i don't feel strongly about it, but i like @hanm 's idea.
---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/545#discussion_r205662651
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -866,6 +866,9 @@ public void setServerCnxnFactory(ServerCnxnFactory
factory) {
}
public ServerCnxnFactory getServerCnxnFactory() {
+if (secureServerCnxnFactory != null) {
+return secureServerCnxnFactory;
+}
return serverCnxnFactory;
}
--- End diff --
Would it be better to, instead of mix `secureServerCnxnFactory` in
`getServerCnxnFactory`, add a separate method `getSecureServerCnxnFactory`?
This also maps well with existing set method `setSecureServerCnxnFactory`.
Caller now has to explicitly call both, which is more work, but makes semantics
more clear.
---
[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...
GitHub user anmolnar opened a pull request: https://github.com/apache/zookeeper/pull/545 ZOOKEEPER-2261 When only secureClientPort is configured connections, configuration, connection_stat_reset, and stats admin commands throw NullPointerException Root cause of the issue is that property getter returns the non-secure ServerCnxnFactory instance always. When Quorum SSL is enabled, we set a separate field which is the secure instance. Property getter should detect the scenario and return the proper instance. First commit contains some refactoring: shuffling the existing ZooKeeperServer tests to relevant places. Second commit is the actual fix + new unit tests. Sorry about indentation changes, but `FileTxnLogTest.java` was indented by 2 spaces instead of 4. You can merge this pull request into a Git repository by running: $ git pull https://github.com/anmolnar/zookeeper ZOOKEEPER-2261 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/545.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #545 commit 1fc048f96db7ac9485508a4179cbe8a1e0ef06d8 Author: Andor Molnar Date: 2018-06-15T15:26:17Z ZOOKEEPER-2261. Refactor ZooKeeperServerTest class and move existing tests to proper places commit b34ba0768bef08a8734b89ba5a872151dc98fd49 Author: Andor Molnar Date: 2018-06-15T15:27:55Z ZOOKEEPER-2261. Return secure factory instance if present + unit tests ---
