[GitHub] zookeeper issue #624: ZOOKEEPER-3108:use a new property server.id in the zoo...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/624
  
thanks for remembering to update the doc @maoling !


---

[GitHub] zookeeper pull request #624: ZOOKEEPER-3108:use a new property server.id in ...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/624#discussion_r217873847
  
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml 
---
@@ -265,18 +266,25 @@ server.3=zoo3:2888:3888
   role="bold">server.id=host:port:port. The parameters 
host and port are straightforward. You attribute 
the
-  server id to each machine by creating a file named
+  server id to each machine by setting server.id
+  to a unique integer for each zookeeper server.To keep backwards 
compatibility,
+  you can still creat a file named
   myid, one for each server, which resides in
   that server's data directory, as specified by the configuration 
file
-  parameter dataDir.
+  parameter dataDir.If the unique 
id is both set in the
+  server.id of zoo.cfg and myid file,the server.id has the priority
--- End diff --

i think we should return an error in this case. this will make debugging 
problems that result from this situation hard.


---

[GitHub] zookeeper pull request #624: ZOOKEEPER-3108:use a new property server.id in ...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/624#discussion_r217873955
  
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml 
---
@@ -265,18 +266,25 @@ server.3=zoo3:2888:3888
   role="bold">server.id=host:port:port. The parameters 
host and port are straightforward. You attribute 
the
-  server id to each machine by creating a file named
+  server id to each machine by setting server.id
+  to a unique integer for each zookeeper server.To keep backwards 
compatibility,
+  you can still creat a file named
--- End diff --

we should also talk about the importance of not changing the id since it 
becomes much easier with the configuration file option. we should have 
documented that anyway, but it becomes much more important with this change 
since the id is decoupled from the data.


---

[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/593#discussion_r212782182
  
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml 
---
@@ -1641,6 +1641,24 @@ server.3=zoo3:2888:3888
 Default is "1".
 
   
+
+  
+networkBufferSize
+
+
+  (Java system property: zookeeper.networkBufferSize)
+
+  New in 3.6.0:
+Sets the socket send and receive buffer size between leader
--- End diff --

nit: buffer size in bytes

it probably should be obvious, but it's worth being extra clear.



---

[GitHub] zookeeper pull request #586: Zookeeper 3105:Character coding problem occur w...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/586#discussion_r212781950
  
--- Diff: src/contrib/zkpython/src/c/zookeeper.c ---
@@ -387,8 +387,8 @@ int parse_acls(struct ACL_vector *acls, PyObject 
*pyacls)
 PyObject *perms = PyDict_GetItemString( a, "perms" );
 #if PY_MAJOR_VERSION >= 3
 acls->data[i].perms = (int32_t)(PyLong_AsLong(perms));
-acls->data[i].id.id = strdup( PyUnicode_AsUnicode( 
PyDict_GetItemString( a, "id" ) ) );
-acls->data[i].id.scheme = strdup( PyUnicode_AsUnicode( 
PyDict_GetItemString( a, "scheme" ) ) );
+acls->data[i].id.id = strdup( PyBytes_AS_STRING( 
PyUnicode_AsASCIIString( PyDict_GetItemString( a, "id" ) ) ) );
--- End diff --

i think we can have id be "أليس" and i believe it will get messed up by 
this code. we need to use UTF-8 encoding. (unfortunately, i'm not familiar 
enough with python C-bindings to know how to do it correctly...


---

[GitHub] zookeeper pull request #605: [ZOOKEEPER-3125] Fixing pzxid consistent issue ...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/605#discussion_r212781611
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -452,7 +452,7 @@ public void testElectionFraud() throws IOException, 
InterruptedException {
 Assert.assertTrue("falseLeader never rejoins the quorum", 
foundFollowing);
 }
 
-private void waitForOne(ZooKeeper zk, States state) throws 
InterruptedException {
--- End diff --

this are in the other change, so we are going to have to re-resolve 
depending on which one goes in.


---

[GitHub] zookeeper issue #606: [ZOOKEEPER-3127] Fixing potential data inconsistency d...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/606
  
+1 thank you @lvfangmin 


---

[GitHub] zookeeper pull request #586: Zookeeper 3105:Character coding problem occur w...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/586#discussion_r207599621
  
--- Diff: src/contrib/zkpython/src/c/zookeeper.c ---
@@ -387,8 +387,8 @@ int parse_acls(struct ACL_vector *acls, PyObject 
*pyacls)
 PyObject *perms = PyDict_GetItemString( a, "perms" );
 #if PY_MAJOR_VERSION >= 3
 acls->data[i].perms = (int32_t)(PyLong_AsLong(perms));
-acls->data[i].id.id = strdup( PyUnicode_AsUnicode( 
PyDict_GetItemString( a, "id" ) ) );
-acls->data[i].id.scheme = strdup( PyUnicode_AsUnicode( 
PyDict_GetItemString( a, "scheme" ) ) );
+acls->data[i].id.id = strdup( PyBytes_AS_STRING( 
PyUnicode_AsASCIIString( PyDict_GetItemString( a, "id" ) ) ) );
--- End diff --

this only works for ascii strings. that should be ok for scheme, but maybe 
not for id.


---

[GitHub] zookeeper pull request #588: [ZOOKEEPER-3109] Avoid long unavailable time du...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/588#discussion_r207597633
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java ---
@@ -86,6 +86,7 @@
 
 QuorumCnxManager manager;
 
+private SyncedLearnerTracker leadingVoteSet;
--- End diff --

it's not totally clear what this is used for. can you comment it?


---

[GitHub] zookeeper pull request #588: [ZOOKEEPER-3109] Avoid long unavailable time du...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/588#discussion_r207597400
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -1165,6 +1165,58 @@ synchronized public long 
startForwarding(LearnerHandler handler,
 }
 // VisibleForTesting
 protected final Set connectingFollowers = new HashSet();
+
+private volatile boolean quitWaitForEpoch = false;
+private volatile long timeStartWaitForEpoch = -1;
+private volatile SyncedLearnerTracker voteSet;
+
+public static final String MIN_TIME_WAIT_FOR_EPOCH = 
"zookeeper.leader.minTimeToWaitForEpoch";
--- End diff --

should comment this. isn't it really MAX_TIME_TO_WAIT_FOR_EPOCH?


---

[GitHub] zookeeper pull request #588: [ZOOKEEPER-3109] Avoid long unavailable time du...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/588#discussion_r207596633
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -1165,6 +1165,58 @@ synchronized public long 
startForwarding(LearnerHandler handler,
 }
 // VisibleForTesting
 protected final Set connectingFollowers = new HashSet();
+
+private volatile boolean quitWaitForEpoch = false;
+private volatile long timeStartWaitForEpoch = -1;
+private volatile SyncedLearnerTracker voteSet;
+
+public static final String MIN_TIME_WAIT_FOR_EPOCH = 
"zookeeper.leader.minTimeToWaitForEpoch";
+private static int minTimeToWaitForEpoch;
+static {
+minTimeToWaitForEpoch = 
Integer.getInteger(MIN_TIME_WAIT_FOR_EPOCH, -1);
+LOG.info(MIN_TIME_WAIT_FOR_EPOCH + " = " + minTimeToWaitForEpoch);
+}
+
+// visible for test
+public static void setMinTimeToWaitForEpoch(int minTimeToWaitForEpoch) 
{
+Leader.minTimeToWaitForEpoch = minTimeToWaitForEpoch;
+LOG.info(MIN_TIME_WAIT_FOR_EPOCH + " set to " + 
minTimeToWaitForEpoch);
+}
+
+/**
+ * Quit condition:
+ *
+ * 1 voter goes to looking again and time waitForEpoch > 
minTimeToWaitForEpoch
+ *
+ * Note: the voter may go to looking again due to:
+ * 1. change mind in the last minute when received a different 
notifications
+ * 2. the leader hadn't started leading when it tried to connect to it.
+ */
+private void quitLeading() {
+synchronized(connectingFollowers) {
+quitWaitForEpoch = true;
+connectingFollowers.notifyAll();
+}
+LOG.info("Quit leading due to disloyal voter.");
--- End diff --

i think this should be warning. it is not expected.


---

[GitHub] zookeeper pull request #584: ZOOKEEPER-3102: Potential race condition when c...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/584#discussion_r205959572
  
--- Diff: src/java/main/org/apache/zookeeper/server/DataTree.java ---
@@ -478,7 +478,10 @@ public void createNode(final String path, byte data[], 
List acl,
 HashSet list = ephemerals.get(ephemeralOwner);
 if (list == null) {
 list = new HashSet();
-ephemerals.put(ephemeralOwner, list);
+HashSet _list;
--- End diff --

we need synchronization to prevent znode changes while the znode is being 
snapshot to disk, but for request processing there is only a single thread that 
does mutations and mutations don't happen while read requests are being 
processed.


---

[GitHub] zookeeper issue #579: [ZOOKEEPER-3095] Connect string fix for non-existent h...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/579
  
hey @mjeelanimsft when i did the commit, your email address came up wrong. 
i think i fixed it, but you might want to check your git config.

thanx for the submission!


---

[GitHub] zookeeper issue #583: [ZOOKEEPER-3104] Fix data inconsistency due to NEWLEAD...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/583
  
ok @lvfangmin i'll commit this once the conflict is resolved.


---

[GitHub] zookeeper issue #563: ZOOKEEPER-3072: Throttle race condition fix

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/563
  
thank you @bothejjms !


---

[GitHub] zookeeper issue #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/548
  
+1 i'll kick jenkins


---

[GitHub] zookeeper pull request #555: ZOOKEEPER-3061: add more details to 'Unhandled ...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/555#discussion_r205669958
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java ---
@@ -792,7 +792,14 @@ public boolean syncFollower(long peerLastZxid, 
ZKDatabase db, Leader leader) {
 txnProposalItr.close();
 }
 } else {
-LOG.warn("Unhandled scenario for peer sid: " +  getSid());
+LOG.warn("Unhandled scenario for peer sid: {} 
maxCommittedLog=0x{}"
--- End diff --

the logging levels are different, and it is nice to have the evaluation 
information with weird corner case scenario. i think it's worth surfacing the 
information here as well.


---

[GitHub] zookeeper issue #560: ZOOKEEPER-3082: Fix server snapshot behavior when out ...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/560
  
+1 i approve. thanx for handling the commit @hanm !


---

[GitHub] zookeeper pull request #563: ZOOKEEPER-3072: Throttle race condition fix

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/563#discussion_r205668951
  
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -1128,9 +1128,9 @@ public void processPacket(ServerCnxn cnxn, ByteBuffer 
incomingBuffer) throws IOE
 Record rsp = processSasl(incomingBuffer,cnxn);
 ReplyHeader rh = new ReplyHeader(h.getXid(), 0, 
KeeperException.Code.OK.intValue());
 cnxn.sendResponse(rh,rsp, "response"); // not sure about 
3rd arg..what is it?
-return;
--- End diff --

it would be nice to keep this return since it matches the handling of the 
other auth logic above.

it would also be nice if this was an

`} else if (h.getType() == OpCode.sasl) {`

clause and the
`}
else {`

was done outside of the if since all the other blocks will have returned. i 
think it makes the logic easier to follow.


---

[GitHub] zookeeper issue #565: ZOOKEEPER-3067: Optionally disable client environment ...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/565
  
+1 let's make this happen


---

[GitHub] zookeeper issue #566: ZOOKEEPER-3062: mention fsync.warningthresholdms in Fi...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/566
  
i agree with @maoling, logging it with every warning seems overkill. the 
rest of the change looks great though. are you ok with removing the extra words 
in the log message?


---

[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205667420
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -102,6 +102,21 @@
 /** Maximum time we allow for elapsed fsync before WARNing */
 private final static long fsyncWarningThresholdMS;
 
+/**
+ * This parameter limit the size of each txnlog to a given limit (KB).
+ * It does not affect how often the system will take a snapshot 
[zookeeper.snapCount]
+ * We roll the txnlog when either of the two limits are reached.
+ * Also since we only roll the logs at transaction boundaries, actual 
file size can exceed
+ * this limit by the maximum size of a serialized transaction.
+ * The feature is disabled by default (-1)
+ */
+public static final String LOG_SIZE_LIMIT = 
"zookeeper.txnlogSizeLimitInKb";
--- End diff --

we should also add this property to the doc.


---

[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205667343
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -102,6 +102,21 @@
 /** Maximum time we allow for elapsed fsync before WARNing */
 private final static long fsyncWarningThresholdMS;
 
+/**
+ * This parameter limit the size of each txnlog to a given limit (KB).
+ * It does not affect how often the system will take a snapshot 
[zookeeper.snapCount]
+ * We roll the txnlog when either of the two limits are reached.
+ * Also since we only roll the logs at transaction boundaries, actual 
file size can exceed
+ * this limit by the maximum size of a serialized transaction.
+ * The feature is disabled by default (-1)
+ */
+public static final String LOG_SIZE_LIMIT = 
"zookeeper.txnlogSizeLimitInKb";
--- End diff --

shouldn't it be txnLogSizeLimitInKb?


---

[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205667305
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -102,6 +102,20 @@
 /** Maximum time we allow for elapsed fsync before WARNing */
 private final static long fsyncWarningThresholdMS;
 
+/**
+ * This parameter limit the size of each txnlog to a given limit (KB).
+ * It does not affect how often the system will take a snapshot
+ * [zookeeper.snapCount]
+ *
+ * The feature is disabled by default (-1)
+ */
+public static final String LOG_SIZE_LIMIT = 
"zookeeper.txnlogSizeLimit";
+
+/**
+ * The actual txnlog size limit in bytes.
+ */
+public static long logSizeLimit = -1;
+
--- End diff --

good points @maoling ! is there a reason these two variables should be 
public? it would be nice to make the variable have a similar name to the 
property.


---

[GitHub] zookeeper issue #573: ZOOKEEPER-3090 continue can be replaced with break

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/573
  
i'll kick it again and see if it works.



---

[GitHub] zookeeper pull request #579: [ZOOKEEPER-3095] Connect string fix for non-exi...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/579#discussion_r205666254
  
--- Diff: src/c/tests/zkServer.sh ---
@@ -77,7 +77,7 @@ fi
 
 if [ "x${base_dir}" == "x" ]
 then
-zk_base="../../"
+zk_base="../../../"
--- End diff --

and is it related to this change? ;)


---

[GitHub] zookeeper pull request #579: [ZOOKEEPER-3095] Connect string fix for non-exi...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/579#discussion_r205666234
  
--- Diff: src/c/tests/zkServer.sh ---
@@ -77,7 +77,7 @@ fi
 
 if [ "x${base_dir}" == "x" ]
 then
-zk_base="../../"
+zk_base="../../../"
--- End diff --

is this correct? how did this work before?


---

[GitHub] zookeeper pull request #579: [ZOOKEEPER-3095] Connect string fix for non-exi...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/579#discussion_r205666190
  
--- Diff: src/c/tests/TestClient.cc ---
@@ -325,7 +326,17 @@ class Zookeeper_simpleSystem : public 
CPPUNIT_NS::TestFixture
 CPPUNIT_ASSERT(zk != 0);
 CPPUNIT_ASSERT(ctx.waitForConnected(zk));
 }
-
+
+/* Checks that a non-existent host will not block the connection*/
+void testNonexistentHost() {
+  char hosts[] = "jimmy:,127.0.0.1:22181";
+  watchctx_t ctx;
--- End diff --

4 space indent


---

[GitHub] zookeeper pull request #579: [ZOOKEEPER-3095] Connect string fix for non-exi...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/579#discussion_r205666180
  
--- Diff: src/c/src/zookeeper.c ---
@@ -842,11 +842,16 @@ static int resolve_hosts(const zhandle_t *zh, const 
char *hosts_in, addrvec_t *a
 }
 
 freeaddrinfo(res0);
-
+next:
 host = strtok_r(0, ",", _last);
 }
 #endif
 }
+if (avec->count == 0) {
+  rc = ZSYSTEMERROR; // not a single host resolved
--- End diff --

4 space indent ;)


---

[GitHub] zookeeper issue #583: [ZOOKEEPER-3104] Fix data inconsistency due to NEWLEAD...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/583
  
@nkalmar i think fixing whitespace is good if it is in proximity of the 
code you are changing, which i think is the case in this diff.

the logic looks good to me! thanx @lvfangmin !

+1


---

[GitHub] zookeeper issue #584: ZOOKEEPER-3102: Potential race condition when create e...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/584
  
i'm not sure this is an issue. there is a single thread 
(FileRequestProcessor) that does all the mutations, so i don't think there is a 
race here. there might be a race in other places, but we need to validate that 
and write a test for it.


---

[GitHub] zookeeper issue #585: Zookeeper 3105:Character coding problem occur when cre...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/585
  
wow! what's going on with this patch? it's huge?


---

[GitHub] zookeeper pull request #545: ZOOKEEPER-2261 When only secureClientPort is co...

[breed]

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

[breed]

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 issue #556: ZOOKEEPER-3074:Flaky test:org.apache.zookeeper.server....

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/556
  
it would be nice to get this in. i'm on vacation right now. @anmolnar or 
@phunt do you have any idea how we can debug what is going on on jenkins?


---

[GitHub] zookeeper issue #554: ZOOKEEPER-3073: fix couple of typos

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/554
  
+1 nice! english si hadr! thank you.


---

[GitHub] zookeeper pull request #561: ZOOKEEPER-3083: Remove some redundant and noisy...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/561#discussion_r201304579
  
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -1047,14 +1047,19 @@ public void processConnectRequest(ServerCnxn cnxn, 
ByteBuffer incomingBuffer) th
 cnxn.disableRecv();
 long sessionId = connReq.getSessionId();
 if (sessionId == 0) {
-LOG.info("Client attempting to establish new session at "
-+ cnxn.getRemoteSocketAddress());
-createSession(cnxn, passwd, sessionTimeout);
+long id = createSession(cnxn, passwd, sessionTimeout);
+LOG.info("Client attempting to establish new session: session 
= 0x"
--- End diff --

shouldn't this also be debug like the other session messages?


---

[GitHub] zookeeper pull request #561: ZOOKEEPER-3083: Remove some redundant and noisy...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/561#discussion_r201303257
  
--- Diff: src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java ---
@@ -124,9 +124,7 @@ private void unregister(String path,ZKMBeanInfo bean) 
throws JMException  {
 return;
 if (!bean.isHidden()) {
 final ObjectName objName = makeObjectName(path, bean);
-if (LOG.isInfoEnabled()) {
-LOG.info("Unregister MBean [{}]", objName);
-}
+LOG.debug("Unregister MBean [{}]", objName);
--- End diff --

any idea why this was in an if clause before. i don't think it was needed, 
but just checking.


---

[GitHub] zookeeper pull request #561: ZOOKEEPER-3083: Remove some redundant and noisy...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/561#discussion_r201304300
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -817,10 +817,12 @@ protected void pRequest(Request request) throws 
RequestProcessorException {
 type = OpCode.error;
 txn = new ErrorTxn(e.code().intValue());
 
-LOG.info("Got user-level KeeperException when 
processing "
-+ request.toString() + " aborting 
remaining multi ops."
-+ " Error Path:" + e.getPath()
-+ " Error:" + e.getMessage());
+if (e.code().intValue() > 
Code.APIERROR.intValue()) {
--- End diff --

this one really seems like a debug level to me. the issue is that the 
client request tried to do something create a node that already exists. these 
are very normal cases that the client is probably expecting (in the case of 
leader election, for example). we shouldn't be logging this kind of expected 
behavior for common operations as INFO


---

[GitHub] zookeeper pull request #561: ZOOKEEPER-3083: Remove some redundant and noisy...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/561#discussion_r201304401
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java ---
@@ -878,10 +880,13 @@ protected void pRequest(Request request) throws 
RequestProcessorException {
 request.getHdr().setType(OpCode.error);
 request.setTxn(new ErrorTxn(e.code().intValue()));
 }
-LOG.info("Got user-level KeeperException when processing "
-+ request.toString()
-+ " Error Path:" + e.getPath()
-+ " Error:" + e.getMessage());
+
+if (e.code().intValue() > Code.APIERROR.intValue()) {
--- End diff --

i feel the same as above here.


---

[GitHub] zookeeper pull request #561: ZOOKEEPER-3083: Remove some redundant and noisy...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/561#discussion_r201304775
  
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -1047,14 +1047,19 @@ public void processConnectRequest(ServerCnxn cnxn, 
ByteBuffer incomingBuffer) th
 cnxn.disableRecv();
 long sessionId = connReq.getSessionId();
 if (sessionId == 0) {
-LOG.info("Client attempting to establish new session at "
-+ cnxn.getRemoteSocketAddress());
-createSession(cnxn, passwd, sessionTimeout);
+long id = createSession(cnxn, passwd, sessionTimeout);
+LOG.info("Client attempting to establish new session: session 
= 0x"
++ Long.toHexString(id)
++ ", zxid = 0x" + 
Long.toHexString(connReq.getLastZxidSeen())
++ ", timeout = " + connReq.getTimeOut()
++ ", address = " + cnxn.getRemoteSocketAddress());
 } else {
 long clientSessionId = connReq.getSessionId();
-LOG.info("Client attempting to renew session 0x"
+LOG.info("Client attempting to renew session: session = 0x"
--- End diff --

i feel this should be debug as well.


---

[GitHub] zookeeper pull request #561: ZOOKEEPER-3083: Remove some redundant and noisy...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/561#discussion_r201303059
  
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -754,7 +754,7 @@ public void finishSessionInit(ServerCnxn cnxn, boolean 
valid) {
 cnxn.sendBuffer(bb);
 
 if (valid) {
-LOG.info("Established session 0x"
+LOG.debug("Established session 0x"
--- End diff --

i think this should go to debug. if you have thousands of clients that are 
reconnecting after a leader change the log fills with these. if you are really 
interested in this line you can add a log4j config that will show it. but this 
really creates a lot of noise in the log.


---

[GitHub] zookeeper issue #556: ZOOKEEPER-3074:Flaky test:org.apache.zookeeper.server....

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/556
  
yes, it did look like they passed, but it stayed as failed. kicking again.


---

[GitHub] zookeeper issue #562: [ZOOKEEPER-3084] Exit when ZooKeeper cannot bind to th...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/562
  
+1 looks like this one is good to go. i'll kick off the tests again. if it 
passes, do you want to commit @hanm , or shall i?


---

[GitHub] zookeeper issue #556: ZOOKEEPER-3074:Flaky test:org.apache.zookeeper.server....

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/556
  
+1 thank you @maoling !

i've restarted the build. if it passes, i'll get this committed.


---

[GitHub] zookeeper issue #558: ZOOKEEPER-3078: remove print_completion_queue

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/558
  
+1 i agree. thanx you. will commit tomorrow if no -1 votes


---

[GitHub] zookeeper pull request #562: [ZOOKEEPER-3084] Exit when zeus cannot bind to ...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/562#discussion_r200717310
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -905,6 +908,12 @@ public void run() {
 + "I won't be able to participate in leader "
 + "election any longer: "
 + self.getElectionAddress());
+if (exitException instanceof BindException) {
+// After leaving listener thread, the host cannot join 
the
+// quorum anymore, this is a severe error that we 
cannot
+// recover from, so we need to exit
+System.exit(14);
--- End diff --

how did we pick 14?


---

[GitHub] zookeeper pull request #560: ZOOKEEPER-3082: Fix server snapshot behavior wh...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/560#discussion_r200716591
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java ---
@@ -399,8 +399,30 @@ public void save(DataTree dataTree,
 File snapshotFile = new File(snapDir, 
Util.makeSnapshotName(lastZxid));
 LOG.info("Snapshotting: 0x{} to {}", Long.toHexString(lastZxid),
 snapshotFile);
-snapLog.serialize(dataTree, sessionsWithTimeouts, snapshotFile, 
syncSnap);
-
+try {
+snapLog.serialize(dataTree, sessionsWithTimeouts, 
snapshotFile, syncSnap);
+} catch (IOException e) {
+if (snapshotFile.length() == 0) {
--- End diff --

this seems extra safe. i'm wondering if perhaps we should always delete the 
snapshot file on an exception. the snapshot file probably will not be valid if 
an exception occurred. right?


---

[GitHub] zookeeper issue #559: ZOOKEEPER-3079: avoid unsafe use of sprintf(3)

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/559
  
+1 thanks for the submission. if there are no objections, i'll commit this 
tomorrow


---

[GitHub] zookeeper issue #353: [ZOOKEEPER-2886] Permanent session moved error in mult...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/353
  
ok, with two +1s i'll be committing this one.


---

[GitHub] zookeeper issue #353: [ZOOKEEPER-2886] Permanent session moved error in mult...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/353
  
quick question, is it ok that the voting happens here rather than the jira? 
i thought comments would be bridged over, but that doesn't seem to be 
happening...


---

[GitHub] zookeeper issue #353: [ZOOKEEPER-2886] Permanent session moved error in mult...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/353
  
+1

thanx @lvfangmin ! this looks good to me. are you okay with this @anmolnar ?


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197933819
  
--- Diff: src/java/test/org/apache/zookeeper/common/NetUtilsTest.java ---
@@ -0,0 +1,46 @@
+package apache.zookeeper.common;
--- End diff --

@mjeelanimsft i think this is why the tests are failing. can you fix this 
package name? (good catch @maoling !)


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194501452
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -64,6 +66,8 @@
 
 protected InetSocketAddress clientPortAddress;
 protected InetSocketAddress secureClientPortAddress;
+protected boolean sslQuorum = false;
+protected boolean shouldUsePortUnification = false;
--- End diff --

it would be nice to expose these through JMX


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194498695
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKConfig.java ---
@@ -107,14 +99,33 @@ private void init() {
  * this configuration.
  */
 protected void handleBackwardCompatibility() {
-properties.put(SSL_KEYSTORE_LOCATION, 
System.getProperty(SSL_KEYSTORE_LOCATION));
-properties.put(SSL_KEYSTORE_PASSWD, 
System.getProperty(SSL_KEYSTORE_PASSWD));
-properties.put(SSL_TRUSTSTORE_LOCATION, 
System.getProperty(SSL_TRUSTSTORE_LOCATION));
-properties.put(SSL_TRUSTSTORE_PASSWD, 
System.getProperty(SSL_TRUSTSTORE_PASSWD));
-properties.put(SSL_AUTHPROVIDER, 
System.getProperty(SSL_AUTHPROVIDER));
 properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
 properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
 properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
+
+ClientX509Util clientX509Util = new ClientX509Util();
--- End diff --

should we have client stuff in this diff? we are just doing quorum 
security. right?


---

[GitHub] zookeeper issue #262: ZOOKEEPER-2789: Reassign `ZXID` for solving 32bit over...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/262
  
i think it would be much better to extend ZOOKEEPER-1277 to more 
transparently do the rollover without a full leader election.

the main issue i have with shortening the epoch size is that once the epoch 
hits the maximum value the ensemble is stuck, nothing can proceed, so we really 
need to keep the epoch size big enough that we would never hit that condition. 
i don't think a 16-bit epoch satisfies that requirement.


---

[GitHub] zookeeper issue #120: ZOOKEEPER-261

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/120
  
it shows up in https://git-wip-us.apache.org/repos/asf?p=zookeeper.git


---
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 #120: ZOOKEEPER-261

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/120
  
hmm, perhaps you are right. i don't think the script is working for me 
properly... checking.


---
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 #120: ZOOKEEPER-261

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/120
  
hmm this is committed. anyone understands why it doesn't autoclose?


---
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 #120: ZOOKEEPER-261

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/120
  
commited. thanx everyone for reviewing and brian for your contribution.


---
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 #120: ZOOKEEPER-261

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/120
  
@eribeiro i think this is ready to go. do you feel that your question has 
been answer sufficiently?


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/90#discussion_r92719995
  
--- Diff: src/c/tests/TestClient.cc ---
@@ -47,6 +47,10 @@ struct buff_struct_2 {
 char *buffer;
 };
 
+// TODO(br33d): the vast majority of this test is not usable with single 
threaded.
--- End diff --

it's more a matter of implementing the tests than refactoring :)


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/117#discussion_r90791441
  
--- 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 --

i think we should skip that test. we have a fix for ZOOKEEPER-261 that 
fixes that specific scenario.


---
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 #117: ZOOKEEPER-2325: Data inconsistency if all snapshots em...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/117
  
@hanm i can't seem to find your comment :)


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/117#discussion_r90523812
  
--- 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 --

[GitHub] zookeeper pull request #117: ZOOKEEPER-2325: Data inconsistency if all snaps...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/117#discussion_r90523518
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java ---
@@ -165,8 +165,22 @@ public File getSnapDir() {
  */
 public long restore(DataTree dt, Map<Long, Integer> 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 */
+if (txnLog.getLastLoggedZxid() != -1) {
+throw new IOException(
+"No snapshot found, but there are log entries. " +
+"Something is broken!");
+}
+/* TODO: (br33d) we should either put a ConcurrentHashMap on 
restore()
+ *   or use Map on save() */
+save(dt, (ConcurrentHashMap<Long, Integer>)sessions);
--- End diff --

yes, ZOOKEEPER-261 is still a problem. is that what you are referring to? 
brian has a patch coming that builds on this one to fix that problem.


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

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/100
  
+1 everything looks good from our side. we don't use that error code.


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

[breed]

GitHub user breed opened a pull request:

https://github.com/apache/zookeeper/pull/117

ZOOKEEPER-2325: Data inconsistency if all snapshots empty or missing



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/breed/zookeeper ZOOKEEPER-2325

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/117.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 #117


commit 02bf3d57786d51da205e78a070a45703da21f916
Author: Benjamin Reed <br...@fb.com>
Date:   2016-11-29T22:08:22Z

ZOOKEEPER-2325: Data inconsistency if all snapshots empty or missing




---
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 #104: ZOOKEEPER-2631: Make issue extraction in the git pull ...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/104
  
+1


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/90#discussion_r87652058
  
--- 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 --

so, just to be clear. is this change correct? we don't need to call 
notify_sync_completion in the non-threaded case. right?


---
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 #102: ZOOKEEPER-2628: Fix findbug warnings.

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/102#discussion_r86902396
  
--- Diff: src/java/main/org/apache/jute/compiler/CGenerator.java ---
@@ -61,70 +61,88 @@ void genCode() throws IOException {
 + outputDirectory);
 }
 }
-FileWriter c = new FileWriter(new File(outputDirectory, 
mName+".c"));
-FileWriter h = new FileWriter(new File(outputDirectory, 
mName+".h"));
 
-h.write("/**\n");
-h.write("* Licensed to the Apache Software Foundation (ASF) under 
one\n");
-h.write("* or more contributor license agreements.  See the NOTICE 
file\n");
-h.write("* distributed with this work for additional 
information\n");
-h.write("* regarding copyright ownership.  The ASF licenses this 
file\n");
-h.write("* to you under the Apache License, Version 2.0 (the\n");
-h.write("* \"License\"); you may not use this file except in 
compliance\n");
-h.write("* with the License.  You may obtain a copy of the License 
at\n");
-h.write("*\n");
-h.write("* http://www.apache.org/licenses/LICENSE-2.0\n;);
-h.write("*\n");
-h.write("* Unless required by applicable law or agreed to in 
writing, software\n");
-h.write("* distributed under the License is distributed on an \"AS 
IS\" BASIS,\n");
-h.write("* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either 
express or implied.\n");
-h.write("* See the License for the specific language governing 
permissions and\n");
-h.write("* limitations under the License.\n");
-h.write("*/\n");
-h.write("\n");
+FileWriter c = null, h = null;
+try {
+c = new FileWriter(new File(outputDirectory, mName + ".c"));
+h = new FileWriter(new File(outputDirectory, mName + ".h"));
 
-c.write("/**\n");
-c.write("* Licensed to the Apache Software Foundation (ASF) under 
one\n");
-c.write("* or more contributor license agreements.  See the NOTICE 
file\n");
-c.write("* distributed with this work for additional 
information\n");
-c.write("* regarding copyright ownership.  The ASF licenses this 
file\n");
-c.write("* to you under the Apache License, Version 2.0 (the\n");
-c.write("* \"License\"); you may not use this file except in 
compliance\n");
-c.write("* with the License.  You may obtain a copy of the License 
at\n");
-c.write("*\n");
-c.write("* http://www.apache.org/licenses/LICENSE-2.0\n;);
-c.write("*\n");
-c.write("* Unless required by applicable law or agreed to in 
writing, software\n");
-c.write("* distributed under the License is distributed on an \"AS 
IS\" BASIS,\n");
-c.write("* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either 
express or implied.\n");
-c.write("* See the License for the specific language governing 
permissions and\n");
-c.write("* limitations under the License.\n");
-c.write("*/\n");
-c.write("\n");
+h.write("/**\n");
+h.write("* Licensed to the Apache Software Foundation (ASF) 
under one\n");
+h.write("* or more contributor license agreements.  See the 
NOTICE file\n");
+h.write("* distributed with this work for additional 
information\n");
+h.write("* regarding copyright ownership.  The ASF licenses 
this file\n");
+h.write("* to you under the Apache License, Version 2.0 
(the\n");
+h.write("* \"License\"); you may not use this file except in 
compliance\n");
+h.write("* with the License.  You may obtain a copy of the 
License at\n");
+h.write("*\n");
+h.write("* http://www.apache.org/licenses/LICENSE-2.0\n;);
+h.write("*\n");
+h.write("* Unless required by applicable law or agreed to in 
writing, software\n");
+h.write("* distributed under the License is distributed on an 
\"AS IS\" BASIS,\n");
+h.write("* WITHOUT WARRANTIES OR

[GitHub] zookeeper issue #97: ZOOKEEPER-2624: Add test script for pull requests

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/97
  
agreed


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/96#discussion_r86679058
  
--- Diff: src/java/test/org/apache/zookeeper/server/DataTreeTest.java ---
@@ -200,29 +198,34 @@ public void 
testSerializeDoesntLockDataNodeWhileWriting() throws Exception {
 BinaryOutputArchive oa = new BinaryOutputArchive(out) {
 @Override
 public void writeRecord(Record r, String tag) throws 
IOException {
-DataNode node = (DataNode) r;
-if (node.data.length == 1 && node.data[0] == 42) {
-final Semaphore semaphore = new Semaphore(0);
-new Thread(new Runnable() {
-@Override
-public void run() {
-synchronized (markerNode) {
-//When we lock markerNode, allow 
writeRecord to continue
-semaphore.release();
+// Need check if the record is a DataNode instance because 
of changes in ZOOKEEPER-2014
+// which adds default ACL to config node.
+if (r instanceof DataNode) {
--- End diff --

is there a reason we added the instanceof here? if we didn't need it 
before, why do we need it now?


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/96#discussion_r86678939
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1108,6 +1109,42 @@ server.3=zoo3:2888:3888
   
 
   
+
+  
+reconfigEnabled
+
+
+  (No Java system property)
+
+  New in 3.5.3:
+This controls the enabling or disabling of
+
+  Dynamic Reconfiguration feature. When the feature
+is enabled, users can perform reconfigure operations 
through
+the ZooKeeper client API or through ZooKeeper command line 
tools
+assuming users are authorized to perform such operations.
+When the feature is disabled, no user, including the super 
user,
+can perform a reconfiguration. Any attempt to reconfigure 
will return an error.
+"reconfigEnabled" option 
can be set as
+"reconfigEnabled=false" or
+"reconfigEnabled=true"
+to a server's config file, or using QuorumPeerConfig's
+setReconfigEnabled method. The default value is false.
+
+If present, the value should be consistent across every 
server in
+the entire ensemble. Setting the value as true on some 
servers and false
+on other servers will cause inconsistent behavior depends 
on which server
--- End diff --

depends -> depending


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/96#discussion_r86679094
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java ---
@@ -0,0 +1,220 @@
+/**
+ * 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.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeoutException;
+
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.admin.ZooKeeperAdmin;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReconfigExceptionTest extends ZKTestCase {
+private static final Logger LOG = LoggerFactory
+.getLogger(ReconfigExceptionTest.class);
+private static String authProvider = 
"zookeeper.DigestAuthenticationProvider.superDigest";
+// Use DigestAuthenticationProvider.base64Encode or
+// run ZooKeeper jar with 
org.apache.zookeeper.server.auth.DigestAuthenticationProvider to generate 
password.
+// An example:
+// java -cp 
zookeeper-3.6.0-SNAPSHOT.jar:lib/log4j-1.2.17.jar:lib/slf4j-log4j12-1.7.5.jar:
+// lib/slf4j-api-1.7.5.jar 
org.apache.zookeeper.server.auth.DigestAuthenticationProvider super:test
+// The password here is 'test'.
+private static String superDigest = 
"super:D/InIHSb7yEEbrWz8b9l71RjZJU=";
+private QuorumUtil qu;
+private ZooKeeperAdmin zkAdmin;
+
+@Before
+public void setup() throws InterruptedException, 
KeeperException.NoNodeException {
--- End diff --

we don't throw NoNodeException anymore


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/96#discussion_r86679075
  
--- Diff: src/java/test/org/apache/zookeeper/test/ClientBase.java ---
@@ -356,7 +356,7 @@ private static int getPort(String hostPort) {
  */
 public static void startServerInstance(File dataDir,
 ServerCnxnFactory factory, String hostPort) throws IOException,
-InterruptedException {
+InterruptedException, KeeperException.NoNodeException {
--- End diff --

this isn't needed anymore right?


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/96#discussion_r86679106
  
--- Diff: 
src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java ---
@@ -0,0 +1,220 @@
+/**
+ * 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.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeoutException;
+
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.admin.ZooKeeperAdmin;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.quorum.QuorumPeerConfig;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReconfigExceptionTest extends ZKTestCase {
+private static final Logger LOG = LoggerFactory
+.getLogger(ReconfigExceptionTest.class);
+private static String authProvider = 
"zookeeper.DigestAuthenticationProvider.superDigest";
+// Use DigestAuthenticationProvider.base64Encode or
+// run ZooKeeper jar with 
org.apache.zookeeper.server.auth.DigestAuthenticationProvider to generate 
password.
+// An example:
+// java -cp 
zookeeper-3.6.0-SNAPSHOT.jar:lib/log4j-1.2.17.jar:lib/slf4j-log4j12-1.7.5.jar:
+// lib/slf4j-api-1.7.5.jar 
org.apache.zookeeper.server.auth.DigestAuthenticationProvider super:test
+// The password here is 'test'.
+private static String superDigest = 
"super:D/InIHSb7yEEbrWz8b9l71RjZJU=";
+private QuorumUtil qu;
+private ZooKeeperAdmin zkAdmin;
+
+@Before
+public void setup() throws InterruptedException, 
KeeperException.NoNodeException {
--- End diff --

+1 awesome work! i only found little nits! thanx for sticking with this!


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

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/96#discussion_r86679034
  
--- 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
+zookeeperZnode.addChild(configChildZookeeper);
+} else {
+LOG.error("There's no /zookeeper znode - this should never 
happen.");
+}
+
+nodes.put(configZookeeper, configDataNode);
+try {
+// Reconfig node is access controlled by default 
(ZOOKEEPER-2014).
+setACL(configZookeeper, ZooDefs.Ids.READ_ACL_UNSAFE, -1);
+} catch (KeeperException.NoNodeException e) {
+LOG.error("Fail to set ACL on {} - this should never happen: 
{}", configZookeeper, e);
--- End diff --

actually if we are asserting above, perhaps we should also assert 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 #97: ZOOKEEPER-2624: Add test script for pull request...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/97#discussion_r86679336
  
--- Diff: src/java/test/bin/test-github-pr.sh ---
@@ -0,0 +1,612 @@
+#!/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 "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
+mkdir -p $PATCH_DIR
+  fi
+
+  ## Obtain PR number and title
+  PULLREQUEST_ID=${GIT_PR_NUMBER}
+  PULLREQUEST_TITLE="${GIT_PR_TITLE}"
+
+  ## Extract jira number from PR title
+  defect=${PULLREQUEST_TITLE%%:*}
+
+  echo "Pull request id: ${PULLREQUEST_ID}"
+  echo "Pull request title: ${PULLREQUEST_TITLE}"
+  echo "Defect number: ${defect}"
+  JIRA_COMMENT="GitHub Pull Request ${PULLREQUEST_NUMBER} Build
+  "
+  ;;
+DEVELOPER)
+  ### Set QABUILD to false to indicate that this script is being run 
by a developer
+  QABUILD=false
+  if [[ $# != 10 ]] ; then
+echo "ERROR: usage $0 DEVELOPER   
 
 "
+cleanupAndExit 0
+  fi
+  PATCH_DIR=$3
+  PATCH_FILE=${PATCH_DIR}/patch
+  curl -L $2.diff > ${PATCH_FILE}
+  ### PATCH_FILE contains the location of the patchfile
+  if [[ ! -e "$PATCH_FILE" ]] ; then
+echo "Unable to locate the patch file $PATCH_FILE"
+cleanupAndExit 0
+  fi
+  ### Check if $PATCH_DIR exists. If it does not exist, create a new 
directory
+  if [[ ! -e "$PATCH_DIR" ]] ; then
+   mkdir "$PATCH_DIR"
+   if [[ $? == 0 ]] ; then
+ echo "$PATCH_DIR has been created"
+   else
+ echo "Unable to create $PATCH_DIR"
+ cleanupAndExit 0
+   fi
+  fi
+  GIT=$4
+  GREP=$5
+  PATCH=$6
+  FINDBUGS_HOME=$7
+  FORREST_HOME=$8
+  BASEDIR=$9
+  JAVA5_HOME=${10}
+  ### Obtain the patch filename to append it to the version number
+  local subject=`grep "Subject:" ${PATCH_FILE}`
+  local length=`expr match ${subject} ZOOKEEPER-[0-9]*`
+  local position=`expr index ${subject} ZOOKEEPER-`
+  defect=${${subject:$position:$length}#ZOOKEEPER-}
+  ;;
+*)
+  echo "ERROR: usage $0 QABUILD [args] | DEVELOPER [args]"
+  cleanupAndExit 0
+  ;;
+  esac
+}
+

+###
+checkout () {
+  echo ""
+  echo ""
+  echo 
"=="
+  echo 
"=="
+  echo "Testing patch for pull request ${PULLREQUEST_ID}."
+  echo 
"=="
+  echo 
"=="
+  echo ""
+  echo ""
+  ### When run by a developer, if the workspace contains modifications, do 
not continue
+  # Ref http://stackoverflow.com/a/2659808 for details on checking d

[GitHub] zookeeper pull request #97: ZOOKEEPER-2624: Add test script for pull request...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/97#discussion_r86679319
  
--- Diff: src/java/test/bin/test-github-pr.sh ---
@@ -0,0 +1,612 @@
+#!/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 "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
+mkdir -p $PATCH_DIR
+  fi
+
+  ## Obtain PR number and title
+  PULLREQUEST_ID=${GIT_PR_NUMBER}
+  PULLREQUEST_TITLE="${GIT_PR_TITLE}"
+
+  ## Extract jira number from PR title
+  defect=${PULLREQUEST_TITLE%%:*}
+
+  echo "Pull request id: ${PULLREQUEST_ID}"
+  echo "Pull request title: ${PULLREQUEST_TITLE}"
+  echo "Defect number: ${defect}"
+  JIRA_COMMENT="GitHub Pull Request ${PULLREQUEST_NUMBER} Build
+  "
+  ;;
+DEVELOPER)
+  ### Set QABUILD to false to indicate that this script is being run 
by a developer
+  QABUILD=false
+  if [[ $# != 10 ]] ; then
+echo "ERROR: usage $0 DEVELOPER   
 
 "
+cleanupAndExit 0
+  fi
+  PATCH_DIR=$3
+  PATCH_FILE=${PATCH_DIR}/patch
+  curl -L $2.diff > ${PATCH_FILE}
+  ### PATCH_FILE contains the location of the patchfile
+  if [[ ! -e "$PATCH_FILE" ]] ; then
+echo "Unable to locate the patch file $PATCH_FILE"
+cleanupAndExit 0
+  fi
+  ### Check if $PATCH_DIR exists. If it does not exist, create a new 
directory
+  if [[ ! -e "$PATCH_DIR" ]] ; then
+   mkdir "$PATCH_DIR"
+   if [[ $? == 0 ]] ; then
+ echo "$PATCH_DIR has been created"
+   else
+ echo "Unable to create $PATCH_DIR"
+ cleanupAndExit 0
+   fi
+  fi
+  GIT=$4
+  GREP=$5
+  PATCH=$6
+  FINDBUGS_HOME=$7
+  FORREST_HOME=$8
+  BASEDIR=$9
+  JAVA5_HOME=${10}
+  ### Obtain the patch filename to append it to the version number
+  local subject=`grep "Subject:" ${PATCH_FILE}`
+  local length=`expr match ${subject} ZOOKEEPER-[0-9]*`
+  local position=`expr index ${subject} ZOOKEEPER-`
+  defect=${${subject:$position:$length}#ZOOKEEPER-}
+  ;;
+*)
+  echo "ERROR: usage $0 QABUILD [args] | DEVELOPER [args]"
+  cleanupAndExit 0
+  ;;
+  esac
+}
+

+###
+checkout () {
+  echo ""
+  echo ""
+  echo 
"=="
+  echo 
"=="
+  echo "Testing patch for pull request ${PULLREQUEST_ID}."
+  echo 
"=="
+  echo 
"=="
+  echo ""
+  echo ""
+  ### When run by a developer, if the workspace contains modifications, do 
not continue
+  # Ref http://stackoverflow.com/a/2659808 for details on checking d

[GitHub] zookeeper pull request #97: ZOOKEEPER-2624: Add test script for pull request...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/97#discussion_r86679277
  
--- Diff: src/java/test/bin/test-github-pr.sh ---
@@ -0,0 +1,612 @@
+#!/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 "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
+mkdir -p $PATCH_DIR
+  fi
+
+  ## Obtain PR number and title
+  PULLREQUEST_ID=${GIT_PR_NUMBER}
+  PULLREQUEST_TITLE="${GIT_PR_TITLE}"
+
+  ## Extract jira number from PR title
+  defect=${PULLREQUEST_TITLE%%:*}
+
+  echo "Pull request id: ${PULLREQUEST_ID}"
+  echo "Pull request title: ${PULLREQUEST_TITLE}"
+  echo "Defect number: ${defect}"
+  JIRA_COMMENT="GitHub Pull Request ${PULLREQUEST_NUMBER} Build
+  "
+  ;;
+DEVELOPER)
+  ### Set QABUILD to false to indicate that this script is being run 
by a developer
+  QABUILD=false
+  if [[ $# != 10 ]] ; then
+echo "ERROR: usage $0 DEVELOPER   
 
 "
+cleanupAndExit 0
+  fi
+  PATCH_DIR=$3
+  PATCH_FILE=${PATCH_DIR}/patch
+  curl -L $2.diff > ${PATCH_FILE}
+  ### PATCH_FILE contains the location of the patchfile
+  if [[ ! -e "$PATCH_FILE" ]] ; then
+echo "Unable to locate the patch file $PATCH_FILE"
+cleanupAndExit 0
+  fi
+  ### Check if $PATCH_DIR exists. If it does not exist, create a new 
directory
+  if [[ ! -e "$PATCH_DIR" ]] ; then
+   mkdir "$PATCH_DIR"
+   if [[ $? == 0 ]] ; then
+ echo "$PATCH_DIR has been created"
+   else
+ echo "Unable to create $PATCH_DIR"
+ cleanupAndExit 0
+   fi
+  fi
+  GIT=$4
+  GREP=$5
+  PATCH=$6
+  FINDBUGS_HOME=$7
+  FORREST_HOME=$8
+  BASEDIR=$9
+  JAVA5_HOME=${10}
+  ### Obtain the patch filename to append it to the version number
+  local subject=`grep "Subject:" ${PATCH_FILE}`
+  local length=`expr match ${subject} ZOOKEEPER-[0-9]*`
+  local position=`expr index ${subject} ZOOKEEPER-`
+  defect=${${subject:$position:$length}#ZOOKEEPER-}
+  ;;
+*)
+  echo "ERROR: usage $0 QABUILD [args] | DEVELOPER [args]"
+  cleanupAndExit 0
+  ;;
+  esac
+}
+

+###
+checkout () {
+  echo ""
+  echo ""
+  echo 
"=="
+  echo 
"=="
+  echo "Testing patch for pull request ${PULLREQUEST_ID}."
+  echo 
"=="
+  echo 
"=="
+  echo ""
+  echo ""
+  ### When run by a developer, if the workspace contains modifications, do 
not continue
+  # Ref http://stackoverflow.com/a/2659808 for details on checking d

[GitHub] zookeeper pull request #97: ZOOKEEPER-2624: Add test script for pull request...

[breed]

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/97#discussion_r86679330
  
--- Diff: src/java/test/bin/test-github-pr.sh ---
@@ -0,0 +1,612 @@
+#!/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 "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
+mkdir -p $PATCH_DIR
+  fi
+
+  ## Obtain PR number and title
+  PULLREQUEST_ID=${GIT_PR_NUMBER}
+  PULLREQUEST_TITLE="${GIT_PR_TITLE}"
+
+  ## Extract jira number from PR title
+  defect=${PULLREQUEST_TITLE%%:*}
+
+  echo "Pull request id: ${PULLREQUEST_ID}"
+  echo "Pull request title: ${PULLREQUEST_TITLE}"
+  echo "Defect number: ${defect}"
+  JIRA_COMMENT="GitHub Pull Request ${PULLREQUEST_NUMBER} Build
+  "
+  ;;
+DEVELOPER)
+  ### Set QABUILD to false to indicate that this script is being run 
by a developer
+  QABUILD=false
+  if [[ $# != 10 ]] ; then
+echo "ERROR: usage $0 DEVELOPER   
 
 "
+cleanupAndExit 0
+  fi
+  PATCH_DIR=$3
+  PATCH_FILE=${PATCH_DIR}/patch
+  curl -L $2.diff > ${PATCH_FILE}
+  ### PATCH_FILE contains the location of the patchfile
+  if [[ ! -e "$PATCH_FILE" ]] ; then
+echo "Unable to locate the patch file $PATCH_FILE"
+cleanupAndExit 0
+  fi
+  ### Check if $PATCH_DIR exists. If it does not exist, create a new 
directory
+  if [[ ! -e "$PATCH_DIR" ]] ; then
+   mkdir "$PATCH_DIR"
+   if [[ $? == 0 ]] ; then
+ echo "$PATCH_DIR has been created"
+   else
+ echo "Unable to create $PATCH_DIR"
+ cleanupAndExit 0
+   fi
+  fi
+  GIT=$4
+  GREP=$5
+  PATCH=$6
+  FINDBUGS_HOME=$7
+  FORREST_HOME=$8
+  BASEDIR=$9
+  JAVA5_HOME=${10}
+  ### Obtain the patch filename to append it to the version number
+  local subject=`grep "Subject:" ${PATCH_FILE}`
+  local length=`expr match ${subject} ZOOKEEPER-[0-9]*`
+  local position=`expr index ${subject} ZOOKEEPER-`
+  defect=${${subject:$position:$length}#ZOOKEEPER-}
+  ;;
+*)
+  echo "ERROR: usage $0 QABUILD [args] | DEVELOPER [args]"
+  cleanupAndExit 0
+  ;;
+  esac
+}
+

+###
+checkout () {
+  echo ""
+  echo ""
+  echo 
"=="
+  echo 
"=="
+  echo "Testing patch for pull request ${PULLREQUEST_ID}."
+  echo 
"=="
+  echo 
"=="
+  echo ""
+  echo ""
+  ### When run by a developer, if the workspace contains modifications, do 
not continue
+  # Ref http://stackoverflow.com/a/2659808 for details on checking d

[GitHub] zookeeper issue #90: ZOOKEEPER-761: Remove *synchronous* calls from the *sin...

[breed]

Github user breed commented on the issue:

https://github.com/apache/zookeeper/pull/90
  
@fpj on the plus side that failure validates that the change is correct :) 
(it is trying to use a sync API with a non threaded test)


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

[breed]

GitHub user breed opened a pull request:

https://github.com/apache/zookeeper/pull/90

ZOOKEEPER-761: Remove *synchronous* calls from the *single-threaded* C 
client API

the synchronous calls from a single-threaded client do not work. this patch
makes using them in a single-threaded client a compilation error.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/breed/zookeeper ZOOKEEPER-761

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/90.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 #90


commit af04dd6fc0b74ab723e6ce449c0e80cc73df
Author: Ben Reed <br...@fb.com>
Date:   2016-10-19T17:36:44Z

ZOOKEEPER-761: Remove *synchronous* calls from the *single-threaded* C 
client API

the synchronous calls from a single-threaded client do not work. this patch
makes using them in a single-threaded client a compilation error.




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