[GitHub] zookeeper issue #522: ZOOKEEPER-1919 Update the C implementation of removeWa...
Github user rgs1 commented on the issue: https://github.com/apache/zookeeper/pull/522 lgtm, r+. thanks! ---
[GitHub] zookeeper pull request #522: ZOOKEEPER-1919 Update the C implementation of r...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/522#discussion_r189469574
--- Diff: src/c/src/zookeeper.c ---
@@ -4502,6 +4532,30 @@ static void process_sync_completion(zhandle_t *zh,
}
}
+static int remove_watches(
+zhandle_t *zh, const char *path, ZooWatcherType wtype,
+watcher_fn watcher, void *wctx, int local, int all)
+{
+int rc = 0;
+ struct sync_completion *sc;
--- End diff --
indentation?
---
[GitHub] zookeeper pull request #522: ZOOKEEPER-1919 Update the C implementation of r...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/522#discussion_r189469556
--- Diff: src/c/src/zookeeper.c ---
@@ -4056,22 +4075,33 @@ int zoo_aremove_watchers(zhandle_t *zh, const char
*path, ZooWatcherType wtype,
oa = create_buffer_oarchive();
rc = serialize_RequestHeader(oa, "header", &h);
-rc = rc < 0 ? rc : serialize_RemoveWatchesRequest(oa, "req", &req);
-if (rc < 0) {
+
+ if (all) {
+ req.remove.path = (char*)server_path;
+ req.remove.type = wtype;
+ rc = rc < 0 ? rc : serialize_RemoveWatchesRequest(oa, "req",
&req.remove);
+} else {
+req.check.path = (char*)server_path;
+req.check.type = wtype;
+rc = rc < 0 ? rc : serialize_CheckWatchesRequest(oa, "req",
&req.check);
+ }
+
+ if (rc < 0) {
--- End diff --
ditto
---
[GitHub] zookeeper pull request #522: ZOOKEEPER-1919 Update the C implementation of r...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/522#discussion_r189469546
--- Diff: src/c/src/zookeeper.c ---
@@ -4056,22 +4075,33 @@ int zoo_aremove_watchers(zhandle_t *zh, const char
*path, ZooWatcherType wtype,
oa = create_buffer_oarchive();
rc = serialize_RequestHeader(oa, "header", &h);
-rc = rc < 0 ? rc : serialize_RemoveWatchesRequest(oa, "req", &req);
-if (rc < 0) {
+
+ if (all) {
+ req.remove.path = (char*)server_path;
+ req.remove.type = wtype;
+ rc = rc < 0 ? rc : serialize_RemoveWatchesRequest(oa, "req",
&req.remove);
+} else {
+req.check.path = (char*)server_path;
+req.check.type = wtype;
+rc = rc < 0 ? rc : serialize_CheckWatchesRequest(oa, "req",
&req.check);
+ }
--- End diff --
indentation looks weird here... tabs?
---
[GitHub] zookeeper pull request #131: ZOOKEEPER-2650: Test Improvement by adding more...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/131#discussion_r93572633
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthTestBase.java
---
@@ -78,15 +78,73 @@ protected String startQuorum(final int serverCount,
return connectStr.toString();
}
-protected int[] startQuorum(final int serverCount, StringBuilder
connectStr,
-Map authConfigs, int authServerCount) throws
IOException {
+/**
+ * Starts the given number of quorum servers and will wait for the
quorum
+ * formation.
+ *
+ * @param serverCount
+ *total server count includes participants + observers
+ * @param ObserverCount
+ *number of observers
+ * @param authConfigs
+ *configuration parameters for authentication
+ * @param authServerCount
+ *number of auth enabled servers
+ * @return client port for the respective servers
+ * @throws IOException
+ */
+protected String startQuorum(final int serverCount, int ObserverCount,
--- End diff --
why is ObserverCount uppercased and also why is not used?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #131: ZOOKEEPER-2650: Test Improvement by adding more...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/131#discussion_r93572708
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthTestBase.java
---
@@ -78,15 +78,73 @@ protected String startQuorum(final int serverCount,
return connectStr.toString();
}
-protected int[] startQuorum(final int serverCount, StringBuilder
connectStr,
-Map authConfigs, int authServerCount) throws
IOException {
+/**
+ * Starts the given number of quorum servers and will wait for the
quorum
+ * formation.
+ *
+ * @param serverCount
+ *total server count includes participants + observers
+ * @param ObserverCount
+ *number of observers
+ * @param authConfigs
+ *configuration parameters for authentication
+ * @param authServerCount
+ *number of auth enabled servers
+ * @return client port for the respective servers
+ * @throws IOException
+ */
+protected String startQuorum(final int serverCount, int ObserverCount,
+Map authConfigs, int authServerCount)
+throws IOException {
+StringBuilder connectStr = new StringBuilder();
+final int[] clientPorts = startQuorum(serverCount, 0, connectStr,
+authConfigs, authServerCount);
+for (int i = 0; i < serverCount; i++) {
+Assert.assertTrue("waiting for server " + i + " being up",
+ClientBase.waitForServerUp("127.0.0.1:" +
clientPorts[i],
+ClientBase.CONNECTION_TIMEOUT));
+}
+return connectStr.toString();
+}
+
+/**
+ * Starts the given number of quorum servers and won't wait for the
quorum
+ * formation.
+ *
+ * @param serverCount
+ *total server count includes participants + observers
+ * @param ObserverCount
+ *number of observers
+ * @param connectStr
+ *connection string where clients can used for connection
+ *establishment
+ * @param authConfigs
+ *configuration parameters for authentication
+ * @param authServerCount
+ *number of auth enabled servers
+ * @return client port for the respective servers
+ * @throws IOException
+ */
+protected int[] startQuorum(final int serverCount, int ObserverCount,
--- End diff --
ditto about variable ObserverCount being uppercased
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #132: ZOOKEEPER-2652: Fix HierarchicalQuorumTest.java
Github user rgs1 commented on the issue: https://github.com/apache/zookeeper/pull/132 r+ â merging --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #99: ZOOKEEPER-2549 Add exception handling to sendResponse
Github user rgs1 commented on the issue: https://github.com/apache/zookeeper/pull/99 @fpj, @hanm could you take a look? I am +1, but another look would be nice. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #117: ZOOKEEPER-2325: Data inconsistency if all snaps...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/117#discussion_r90761121
--- Diff:
src/java/test/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java ---
@@ -0,0 +1,134 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import java.io.IOException;
+import java.io.File;
+import java.io.PrintWriter;
+import java.util.List;
+import java.util.LinkedList;
+
+import org.apache.log4j.Logger;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.server.quorum.Leader.Proposal;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.SyncRequestProcessor;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.junit.Assert;
+import org.junit.Test;
+
+/** If snapshots are corrupted to the empty file or deleted, Zookeeper
should
+ * not proceed to read its transactiong log files
+ * Test that zxid == -1 in the presence of emptied/deleted snapshots
+ */
+public class EmptiedSnapshotRecoveryTest extends ZKTestCase implements
Watcher {
+private static final Logger LOG =
Logger.getLogger(RestoreCommittedLogTest.class);
+private static String HOSTPORT = "127.0.0.1:" +
PortAssignment.unique();
+private static final int CONNECTION_TIMEOUT = 3000;
+private static final int N_TRANSACTIONS = 150;
+private static final int SNAP_COUNT = 100;
+
+public void runTest(boolean leaveEmptyFile) throws Exception {
--- End diff --
@breed do you want to take @hanm's suggestion or should I merge this and we
get that in another pass?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #117: ZOOKEEPER-2325: Data inconsistency if all snaps...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/117#discussion_r90491384
--- Diff:
src/java/test/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java ---
@@ -0,0 +1,134 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.test;
+
+import java.io.IOException;
+import java.io.File;
+import java.io.PrintWriter;
+import java.util.List;
+import java.util.LinkedList;
+
+import org.apache.log4j.Logger;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.server.quorum.Leader.Proposal;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.SyncRequestProcessor;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.junit.Assert;
+import org.junit.Test;
+
+/** If snapshots are corrupted to the empty file or deleted, Zookeeper
should
+ * not proceed to read its transactiong log files
+ * Test that zxid == -1 in the presence of emptied/deleted snapshots
+ */
+public class EmptiedSnapshotRecoveryTest extends ZKTestCase implements
Watcher {
+private static final Logger LOG =
Logger.getLogger(RestoreCommittedLogTest.class);
+private static String HOSTPORT = "127.0.0.1:" +
PortAssignment.unique();
+private static final int CONNECTION_TIMEOUT = 3000;
+private static final int N_TRANSACTIONS = 150;
+private static final int SNAP_COUNT = 100;
+
+public void runTest(boolean leaveEmptyFile) throws Exception {
+File tmpSnapDir = ClientBase.createTmpDir();
+File tmpLogDir = ClientBase.createTmpDir();
+ClientBase.setupTestEnv();
+ZooKeeperServer zks = new ZooKeeperServer(tmpSnapDir, tmpLogDir,
3000);
+SyncRequestProcessor.setSnapCount(SNAP_COUNT);
+final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
+ServerCnxnFactory f = ServerCnxnFactory.createFactory(PORT, -1);
+f.startup(zks);
+Assert.assertTrue("waiting for server being up ",
+ClientBase.waitForServerUp(HOSTPORT,CONNECTION_TIMEOUT));
+ZooKeeper zk = new ZooKeeper(HOSTPORT, CONNECTION_TIMEOUT, this);
+try {
+for (int i = 0; i< N_TRANSACTIONS; i++) {
+zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+}
+} finally {
+zk.close();
+}
+f.shutdown();
+zks.shutdown();
+Assert.assertTrue("waiting for server to shutdown",
+ClientBase.waitForServerDown(HOSTPORT,
CONNECTION_TIMEOUT));
+
+// start server again with intact database
+zks = new ZooKeeperServer(tmpSnapDir, tmpLogDir, 3000);
+zks.startdata();
+long zxid = zks.getZKDatabase().getDataTreeLastProcessedZxid();
+LOG.info("After clean restart, zxid = " + zxid);
+Assert.assertTrue("zxid > 0", zxid > 0);
+zks.shutdown();
+
+// Make all snapshots empty
+FileTxnSnapLog txnLogFactory = zks.getTxnLogFactory();
+List snapshots = txnLogFactory.findNRecentSnapshots(10);
+Assert.assertTrue("We have a snapshot to corrupt",
snapshots.size() > 0);
+for (File file: snapshots) {
+if (leaveEmptyFile) {
+new PrintWriter(file).close ();
+}
+else {
--- End diff --
nit:
[GitHub] zookeeper pull request #117: ZOOKEEPER-2325: Data inconsistency if all snaps...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/117#discussion_r90490925
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java ---
@@ -165,8 +165,22 @@ public File getSnapDir() {
*/
public long restore(DataTree dt, Map sessions,
PlayBackListener listener) throws IOException {
-snapLog.deserialize(dt, sessions);
+long deserializeResult = snapLog.deserialize(dt, sessions);
FileTxnLog txnLog = new FileTxnLog(dataDir);
+if (-1L == deserializeResult) {
+/* this means that we couldn't find any snapshot, so we need to
+ * initialize an empty database */
--- End diff --
nit: can you add a reference to ZOOKEEPER-2325 here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/98#discussion_r88602850
--- Diff: src/java/test/org/apache/zookeeper/test/QuorumMajorityTest.java
---
@@ -36,7 +38,30 @@
/***/
@Test
public void testMajQuorums() throws Throwable {
-
+LOG.info("Verify QuorumPeer#electionTimeTaken jmx bean attribute");
+
+ArrayList peers = getPeerList();
+for (int i = 1; i <= peers.size(); i++) {
+QuorumPeer qp = peers.get(i - 1);
+Long electionTimeTaken = -1L;
+String bean = "";
+if (qp.getPeerState() == ServerState.FOLLOWING) {
+bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" +
i
++ ",name1=replica." + i + ",name2=Follower";
+electionTimeTaken = (Long) JMXEnv.ensureBeanAttribute(bean,
+"ElectionTimeTaken");
+Assert.assertTrue("Wrong electionTimeTaken value!",
+electionTimeTaken >= 0);
+} else if (qp.getPeerState() == ServerState.LEADING) {
+bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" +
i
++ ",name1=replica." + i + ",name2=Leader";
--- End diff --
ditto
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/98#discussion_r88602842
--- Diff: src/java/test/org/apache/zookeeper/test/QuorumMajorityTest.java
---
@@ -36,7 +38,30 @@
/***/
@Test
public void testMajQuorums() throws Throwable {
-
+LOG.info("Verify QuorumPeer#electionTimeTaken jmx bean attribute");
+
+ArrayList peers = getPeerList();
+for (int i = 1; i <= peers.size(); i++) {
+QuorumPeer qp = peers.get(i - 1);
+Long electionTimeTaken = -1L;
+String bean = "";
+if (qp.getPeerState() == ServerState.FOLLOWING) {
+bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" +
i
++ ",name1=replica." + i + ",name2=Follower";
--- End diff --
hyper nit: `String.format()` reads better than `+`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #90: ZOOKEEPER-761: Remove *synchronous* calls from t...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/90#discussion_r88183747
--- Diff: src/c/src/zookeeper.c ---
@@ -4052,6 +3958,76 @@ int zoo_amulti(zhandle_t *zh, int count, const
zoo_op_t *ops,
return (rc < 0) ? ZMARSHALLINGERROR : ZOK;
}
+
+int zoo_aremove_watchers(zhandle_t *zh, const char *path, ZooWatcherType
wtype,
+watcher_fn watcher, void *watcherCtx, int local,
+void_completion_t *completion, const void *data)
+{
+char *server_path = prepend_string(zh, path);
+int rc;
+struct oarchive *oa;
+struct RequestHeader h = { get_xid(), ZOO_REMOVE_WATCHES };
+struct RemoveWatchesRequest req = { (char*)server_path, wtype };
+watcher_deregistration_t *wdo;
+
+if (!zh || !isValidPath(server_path, 0)) {
+rc = ZBADARGUMENTS;
+goto done;
+}
+
+if (!local && is_unrecoverable(zh)) {
+rc = ZINVALIDSTATE;
+goto done;
+}
+
+if (!pathHasWatcher(zh, server_path, wtype, watcher, watcherCtx)) {
+rc = ZNOWATCHER;
+goto done;
+}
+
+if (local) {
+removeWatchers(zh, server_path, wtype, watcher, watcherCtx);
+#ifdef THREADED
+notify_sync_completion((struct sync_completion *)data);
--- End diff --
@breed @fpj btw -- sorry for the confusing code. `zoo_aremove_watchers` is
sui generis given that it's the only public method that can return `ZOK`
without scheduling a remote call (for which then, the callback would be
naturally dispatched). Thus, this horrible hack of calling
`notify_sync_completion()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C client.
Github user rgs1 commented on the issue: https://github.com/apache/zookeeper/pull/100 Thanks @hanm ! Lets wait until Monday to see what @breed thinks and then I am happy to merge this. What do you think? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86679205
--- Diff:
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -842,7 +868,26 @@ private void addCnxn(NIOServerCnxn cnxn) {
protected NIOServerCnxn createConnection(SocketChannel sock,
SelectionKey sk, SelectorThread selectorThread) throws
IOException {
-return new NIOServerCnxn(zkServer, sock, sk, this, selectorThread);
+
+NIOServerCnxn cnxn = null;
+
+if (serverCnxnClassCtr != null) {
--- End diff --
what's wrong with setting serverCnxnClassCtr to NettyServerCnxn by default
(by doing all the reflection magic)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86679164
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -716,7 +716,12 @@ public void process(WatchedEvent event) {
// Convert WatchedEvent to a type that can be sent over the wire
WatcherEvent e = event.getWrapper();
-sendResponse(h, e, "notification");
+try {
+sendResponse(h, e, "notification");
+} catch (IOException ex) {
+LOG.debug("Problem sending to " + getRemoteSocketAddress(),
ex);
+close();
--- End diff --
hmm, that would be a pretty bad leak given all what goes on in close():
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #97: ZOOKEEPER-2624: Add test script for pull requests
Github user rgs1 commented on the issue: https://github.com/apache/zookeeper/pull/97 @hanm @fpj i say we merge it and keep iterating; it'll be easier to get it right once we are using it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #97: ZOOKEEPER-2624: Add test script for pull request...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/97#discussion_r86679093
--- Diff: src/java/test/bin/test-github-pr.sh ---
@@ -0,0 +1,605 @@
+#!/usr/bin/env bash
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+#set -x
+
+### Setup some variables.
+### GIT_COMMIT and BUILD_URL are set by Hudson if it is run by patch
process
+### Read variables from properties file
+. `dirname $0`/test-patch.properties
+
+###
+parseArgs() {
+ case "$1" in
+QABUILD)
+ ### Set QABUILD to true to indicate that this script is being run by
Hudson
+ QABUILD=true
+ if [[ $# != 14 ]] ; then
+echo "Number of given parameters: $#"
+echo "ERROR: usage $0 QABUILD
"
+cleanupAndExit 0
+ fi
+ PATCH_DIR=$2
+ PS=$3
+ WGET=$4
+ JIRACLI=$5
+ GIT=$6
+ GREP=$7
+ PATCH=$8
+ FINDBUGS_HOME=$9
+ FORREST_HOME=${10}
+ BASEDIR=${11}
+ JIRA_PASSWD=${12}
+ JAVA5_HOME=${13}
+ CURL=${14}
+ if [ ! -e "$PATCH_DIR" ] ; then
--- End diff --
nit: use `[[ ! -e "$PATCH_DIR" ]]` as you do in other places
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #96: ZOOKEEPER-2014: Only admin should be allowed to ...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/96#discussion_r86673489
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -245,15 +245,25 @@ public DataTree() {
addConfigNode();
}
- public void addConfigNode() {
-DataNode zookeeperZnode = nodes.get(procZookeeper);
- if (zookeeperZnode!=null) { // should always be the case
-zookeeperZnode.addChild(configChildZookeeper);
- } else {
-LOG.error("There's no /zookeeper znode - this should never
happen");
- }
- nodes.put(configZookeeper, configDataNode);
- }
+public void addConfigNode() {
+DataNode zookeeperZnode = nodes.get(procZookeeper);
+if (zookeeperZnode!=null) { // should always be the case
--- End diff --
i think this should be an assertion (and we can drop the LOG.error call)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #96: ZOOKEEPER-2014: Only admin should be allowed to ...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/96#discussion_r86673398
--- Diff: src/c/tests/TestReconfigServer.cc ---
@@ -324,4 +336,109 @@ testRemoveConnectedFollower() {
zookeeper_close(zk);
}
+/**
+ * ZOOKEEPER-2014: only admin or users who are explicitly granted
permission can do reconfig.
+ */
+void TestReconfigServer::
+testReconfigFailureWithoutAuth() {
+std::vector servers;
+std::string version;
+struct Stat stat;
+int len = 1024;
+char buf[len];
+
+// connect to a follower.
+int32_t leader = getLeader();
+std::vector followers = getFollowers();
+CPPUNIT_ASSERT(leader >= 0);
+CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1, (uint32_t)(followers.size()));
+std::stringstream ss;
+for (int i = 0; i < followers.size(); i++) {
+ ss << cluster_[followers[i]]->getHostPort() << ",";
+}
+ss << cluster_[leader]->getHostPort();
+std::string hosts = ss.str().c_str();
+zoo_deterministic_conn_order(true);
+zhandle_t* zk = zookeeper_init(hosts.c_str(), NULL, 1, NULL, NULL,
0);
+CPPUNIT_ASSERT_EQUAL(true, waitForConnected(zk, 10));
+
+std::string connectedHost(zoo_get_current_server(zk));
+std::string portString = connectedHost.substr(connectedHost.find(":")
+ 1);
+uint32_t port;
+std::istringstream (portString) >> port;
+CPPUNIT_ASSERT_EQUAL(cluster_[followers[0]]->getClientPort(), port);
+
+// remove the follower.
+len = 1024;
+ss.str("");
+ss << followers[0];
+// No auth, should fail.
+CPPUNIT_ASSERT_EQUAL((int)ZNOAUTH, zoo_reconfig(zk, NULL,
ss.str().c_str(), NULL, -1, buf, &len, &stat));
+// Wrong auth, should fail.
+CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_add_auth(zk, "digest",
"super:wrong", 11, NULL,(void*)ZOK));
+CPPUNIT_ASSERT_EQUAL((int)ZNOAUTH, zoo_reconfig(zk, NULL,
ss.str().c_str(), NULL, -1, buf, &len, &stat));
+// Right auth, should pass.
+CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_add_auth(zk, "digest",
"super:test", 10, NULL,(void*)ZOK));
+CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_reconfig(zk, NULL,
ss.str().c_str(), NULL, -1, buf, &len, &stat));
+CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat));
+parseConfig(buf, len, servers, version);
+CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1, (uint32_t)(servers.size()));
+for (int i = 0; i < cluster_.size(); i++) {
+if (i == followers[0]) {
+continue;
+}
+CPPUNIT_ASSERT(std::find(servers.begin(), servers.end(),
+ cluster_[i]->getServerString()) != servers.end());
+}
+zookeeper_close(zk);
+}
+
+void TestReconfigServer::
+testReconfigFailureWithoutServerSuperuserPasswordConfigured() {
+std::vector servers;
+std::string version;
+struct Stat stat;
+int len = 1024;
+char buf[len];
+
+// Create a new quorum with the super user's password not configured.
+tearDown();
+ZooKeeperQuorumServer::tConfigPairs configs;
+configs.push_back(std::make_pair("reconfigEnabled", "true"));
+cluster_ = ZooKeeperQuorumServer::getCluster(NUM_SERVERS, configs, "");
+
+// connect to a follower.
+int32_t leader = getLeader();
+std::vector followers = getFollowers();
+CPPUNIT_ASSERT(leader >= 0);
+CPPUNIT_ASSERT_EQUAL(NUM_SERVERS - 1, (uint32_t)(followers.size()));
+std::stringstream ss;
+for (int i = 0; i < followers.size(); i++) {
+ ss << cluster_[followers[i]]->getHostPort() << ",";
+}
+ss << cluster_[leader]->getHostPort();
+std::string hosts = ss.str().c_str();
+zoo_deterministic_conn_order(true);
+zhandle_t* zk = zookeeper_init(hosts.c_str(), NULL, 1, NULL, NULL,
0);
+CPPUNIT_ASSERT_EQUAL(true, waitForConnected(zk, 10));
+
+std::string connectedHost(zoo_get_current_server(zk));
+std::string portString = connectedHost.substr(connectedHost.find(":")
+ 1);
+uint32_t port;
+std::istringstream (portString) >> port;
+CPPUNIT_ASSERT_EQUAL(cluster_[followers[0]]->getClientPort(), port);
--- End diff --
connect to follower seems repeated, can we move it to a private helper
method?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #96: ZOOKEEPER-2014: Only admin should be allowed to ...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/96#discussion_r86673475
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -245,15 +245,25 @@ public DataTree() {
addConfigNode();
}
- public void addConfigNode() {
-DataNode zookeeperZnode = nodes.get(procZookeeper);
- if (zookeeperZnode!=null) { // should always be the case
-zookeeperZnode.addChild(configChildZookeeper);
- } else {
-LOG.error("There's no /zookeeper znode - this should never
happen");
- }
- nodes.put(configZookeeper, configDataNode);
- }
+public void addConfigNode() {
+DataNode zookeeperZnode = nodes.get(procZookeeper);
+if (zookeeperZnode!=null) { // should always be the case
--- End diff --
nit: spaces around `!=`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C cli...
Github user rgs1 commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/100#discussion_r86673221 --- Diff: src/c/src/zookeeper.c --- @@ -1702,15 +1703,9 @@ static void handle_error(zhandle_t *zh,int rc) } cleanup_bufs(zh,1,rc); zh->fd = -1; - + --- End diff -- nit: drop the whitespace that was introduced --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #100: ZOOKEEPER-2627:Remove ZRWSERVERFOUND from C cli...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/100#discussion_r86673245
--- Diff: src/c/src/zookeeper.c ---
@@ -2072,6 +2076,11 @@ static int ping_rw_server(zhandle_t* zh)
out:
close(sock);
+if (!rc) {
+addr_rw_server = 0;
+} else {
+addr_rw_server = &zh->addr_rw_server;
+}
--- End diff --
nit:
```
addr_rw_server = rc ? &zh->addr_rw_server : 0;
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86672337
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -716,7 +716,12 @@ public void process(WatchedEvent event) {
// Convert WatchedEvent to a type that can be sent over the wire
WatcherEvent e = event.getWrapper();
-sendResponse(h, e, "notification");
+try {
+sendResponse(h, e, "notification");
+} catch (IOException ex) {
+LOG.debug("Problem sending to " + getRemoteSocketAddress(),
ex);
--- End diff --
i have mixed feelings with concatenating strings in a hot path (IOException
happening here is a hot path when, for instance, a network blip happens).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86672419
--- Diff:
src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -105,8 +115,26 @@ public void channelConnected(ChannelHandlerContext ctx,
LOG.trace("Channel connected " + e);
}
-NettyServerCnxn cnxn = new NettyServerCnxn(ctx.getChannel(),
-zkServer, NettyServerCnxnFactory.this);
+NettyServerCnxn cnxn = null;
+if (serverCnxnClassCtr != null) {
--- End diff --
same as for NIO, lets set serverCnxnClassCtr to NettyServerCnxn by default
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86672349
--- Diff:
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -630,6 +640,22 @@ public static ByteBuffer getDirectBuffer() {
* limits of the operating system). startup(zks) must be called
subsequently.
*/
public NIOServerCnxnFactory() {
+String serverCnxnClassName =
System.getProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN);
+if ( serverCnxnClassName != null ) {
--- End diff --
nit: extra spaces
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86672326
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -716,7 +716,12 @@ public void process(WatchedEvent event) {
// Convert WatchedEvent to a type that can be sent over the wire
WatcherEvent e = event.getWrapper();
-sendResponse(h, e, "notification");
+try {
+sendResponse(h, e, "notification");
+} catch (IOException ex) {
+LOG.debug("Problem sending to " + getRemoteSocketAddress(),
ex);
+close();
--- End diff --
this is an interesting change -- how/when were we closing the connection
before?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86672385
--- Diff:
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -842,7 +868,26 @@ private void addCnxn(NIOServerCnxn cnxn) {
protected NIOServerCnxn createConnection(SocketChannel sock,
SelectionKey sk, SelectorThread selectorThread) throws
IOException {
-return new NIOServerCnxn(zkServer, sock, sk, this, selectorThread);
+
+NIOServerCnxn cnxn = null;
+
+if (serverCnxnClassCtr != null) {
--- End diff --
why not set serverCnxnClassCtr to NIOServerCnxn by default to avoid looking
up the right constructor every time we want to create a new connection (a bit
of a hot path)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request: refactor(ClientCnxn): no need to gate chea...
Github user rgs1 closed the pull request at: https://github.com/apache/zookeeper/pull/38 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request: refactor(ClientCnxn): no need to gate chea...
GitHub user rgs1 opened a pull request: https://github.com/apache/zookeeper/pull/38 refactor(ClientCnxn): no need to gate cheap LOG.debug calls If building the parameters for LOG.debug is cheap, there is no point in cluttering the code with if (LOG.isDebugEnabled) gates. Thus, this removes the gating when parameters are cheap. Also, some small grammar nits are taken care of plus a few changes of extrapolation instead of concatenation. Note that for hot paths, where we don't want to be creating many strings, the if (LOG.isDebugEnabled) is left untouched. Besides the small variations in the strings, nothing else should change. Signed-off-by: Raul Gutierrez S You can merge this pull request into a Git repository by running: $ git pull https://github.com/rgs1/zookeeper remove-is-debug-enabled-when-call-is-cheap Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/38.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 #38 commit 4d13c3402bdfd8d445496c20b0e286594fdd5021 Author: Raul Gutierrez S Date: 2015-07-13T03:19:20Z refactor(ClientCnxn): no need to gate cheap LOG.debug calls If building the parameters for LOG.debug is cheap, there is no point in cluttering the code with if (LOG.isDebugEnabled) gates. Thus, this removes the gating when parameters are cheap. Also, some small grammar nits are taken care of plus a few changes of extrapolation instead of concatenation. Note that for hot paths, where we don't want to be creating many strings, the if (LOG.isDebugEnabled) is left untouched. Besides the small variations in the strings, nothing else should change. Signed-off-by: Raul Gutierrez S --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request: ZOOKEEPER-2186: QuorumCnxManager#receiveCo...
Github user rgs1 closed the pull request at: https://github.com/apache/zookeeper/pull/30 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request: ZOOKEEPER-2186: QuorumCnxManager#receiveCo...
GitHub user rgs1 opened a pull request: https://github.com/apache/zookeeper/pull/30 ZOOKEEPER-2186: QuorumCnxManager#receiveConnection may crash with ran⦠â¦dom input You can merge this pull request into a Git repository by running: $ git pull https://github.com/rgs1/zookeeper ZOOKEEPER-2186 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/30.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 #30 commit ff9cdc570fb4c84acf3c8b0c64875b5e54d82523 Author: Raul Gutierrez S Date: 2015-05-11T17:48:47Z ZOOKEEPER-2186: QuorumCnxManager#receiveConnection may crash with random input --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
