[GitHub] zookeeper issue #513: ZOOKEEPER-2982: Re-try DNS hostname -> IP resolution i...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/513 The reviews and approvals for this change are in pull request #468 . The intention of this PR was to rebase it to master, while PR #468 targeted branch-3.5. ---
[GitHub] zookeeper issue #468: ZOOKEEPER-2982: Re-try DNS hostname -> IP resolution i...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/468 Merged to branch 3.5 as part of merging pull request #513, which targeted master. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186759876
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
--- End diff --
Ok.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186690327
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
+}
+Collections.shuffle(resolvedAddresses);
--- End diff --
Ok.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186690242
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
--- End diff --
I think it is fine to loop indefinitely because the client needs it for
progress. We need a way to break the loop in the case the client closes, though.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r186689865 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- My preference is the following: 1- That we do not make this API change. It is not necessary to solve the problem of this issue. 2- That we discuss separately whether we want to change the behaviour of the `next()` in the `HostProvider` interface. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186553219
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
+}
+Collections.shuffle(resolvedAddresses);
--- End diff --
It doesn't't hurt, but do we really need to shuffle the resolved addresses?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r186546855 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- It feels odd to throw unknown host here because `next` is supposed to return a resolved host. The caller is asking for next resolved, not to resolve a given host name. Why do we need to throw it here and assuming we do need to throw something here, is there an exception that captures better the error you are trying to propagate? ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186553004
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
--- End diff --
Should we try with the next on the list rather than throwing? After all,
the call is to get the "next" resolved address. Of course, it is possible that
we go through the whole list an find nothing. We might want to throw then so
that we don't loop forever.
---
[GitHub] zookeeper issue #456: ZOOKEEPER-2930: Leader cannot be elected due to networ...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/456 I have also tested this PR a bit locally. ---
[GitHub] zookeeper pull request #456: ZOOKEEPER-2930: Leader cannot be elected due to...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/456#discussion_r168620297
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -318,76 +318,167 @@ public Thread newThread(Runnable r) {
*/
public void testInitiateConnection(long sid) throws Exception {
LOG.debug("Opening channel to server " + sid);
-Socket sock = new Socket();
-setSockOpts(sock);
-sock.connect(self.getVotingView().get(sid).electionAddr, cnxTO);
-initiateConnection(sock, sid);
+initiateConnection(sid,
self.getVotingView().get(sid).electionAddr);
+}
+
+private Socket openChannel(long sid, InetSocketAddress electionAddr) {
+LOG.debug("Opening channel to server " + sid);
+try {
+final Socket sock = new Socket();
+setSockOpts(sock);
+sock.connect(electionAddr, cnxTO);
+LOG.debug("Connected to server " + sid);
+return sock;
+} catch (UnresolvedAddressException e) {
+// Sun doesn't include the address that causes this
+// exception to be thrown, also UAE cannot be wrapped cleanly
+// so we log the exception in order to capture this critical
+// detail.
+LOG.warn("Cannot open channel to " + sid
++ " at election address " + electionAddr, e);
+throw e;
+} catch (IOException e) {
+LOG.warn("Cannot open channel to " + sid
++ " at election address " + electionAddr,
+e);
+return null;
+}
}
/**
* If this server has initiated the connection, then it gives up on the
* connection if it loses challenge. Otherwise, it keeps the
connection.
*/
-public void initiateConnection(final Socket sock, final Long sid) {
+public boolean initiateConnection(final Long sid, InetSocketAddress
electionAddr) {
try {
-startConnection(sock, sid);
-} catch (IOException e) {
-LOG.error("Exception while connecting, id: {}, addr: {},
closing learner connection",
-new Object[] { sid, sock.getRemoteSocketAddress() },
e);
-closeSocket(sock);
-return;
+Socket sock = openChannel(sid, electionAddr);
+if (sock != null) {
+try {
+startConnection(sock, sid);
+} catch (IOException e) {
+LOG.error("Exception while connecting, id: {}, addr:
{}, closing learner connection",
+new Object[]{sid,
sock.getRemoteSocketAddress()}, e);
+closeSocket(sock);
+}
+return true;
+} else {
+return false;
+}
+} finally {
+inprogressConnections.remove(sid);
}
}
-/**
- * Server will initiate the connection request to its peer server
- * asynchronously via separate connection thread.
- */
-public void initiateConnectionAsync(final Socket sock, final Long sid)
{
+synchronized private void connectOneAsync(final Long sid, final
ZooKeeperThread connectorThread) {
+if (senderWorkerMap.get(sid) != null) {
+LOG.debug("There is a connection already for server " + sid);
+return;
+}
if(!inprogressConnections.add(sid)){
// simply return as there is a connection request to
// server 'sid' already in progress.
LOG.debug("Connection request to server id: {} is already in
progress, so skipping this request",
sid);
-closeSocket(sock);
return;
}
try {
-connectionExecutor.execute(
-new QuorumConnectionReqThread(sock, sid));
+connectionExecutor.execute(connectorThread);
connectionThreadCnt.incrementAndGet();
} catch (Throwable e) {
// Imp: Safer side catching all type of exceptions and remove
'sid'
// from inprogress connections. This is to avoid blocking
further
// connection requests from this 'sid' in case of errors.
inprogressConnections.remove(sid);
LOG.error("Exception while submitting quorum connecti
[GitHub] zookeeper pull request #167: ZOOKEEPER-2684 commitProcessor does not crash w...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/167#discussion_r121006265
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java ---
@@ -254,24 +254,23 @@ public void run() {
// If session queue != null, then it is also not
empty.
Request topPending = sessionQueue.poll();
if (request.cxid != topPending.cxid) {
-LOG.error(
-"Got cxid 0x"
-+
Long.toHexString(request.cxid)
-+ " expected 0x" +
Long.toHexString(
-topPending.cxid)
-+ " for client session id "
-+ Long.toHexString(request.sessionId));
-throw new IOException("Error: unexpected cxid
for"
-+ "client session");
+// we can get commit requests that is not at
the queue head when
+// session moves (see ZOOKEEPER-2684). We will
just pass the
+// commit to the next processor and put the
pending back with
+// a warning, we should not see this often
under normal load
+LOG.warn("Got request " + request +
+" but we are expecting request " +
topPending);
+sessionQueue.addFirst(topPending);
+} else {
--- End diff --
Is it the case that for a given session, once we execute the else block
once, executing the if block would be incorrect? If so, would it make sense to
have a flag per session indicating that the else block has not been executed
for the session? It might not even be a flag per session, but perhaps a set of
session ids instead that we remove from once we execute the else block.
---
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 #127: ZOOKEEPER-2646: Java target in branch 3.4 doesn...
Github user fpj closed the pull request at: https://github.com/apache/zookeeper/pull/127 --- 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 #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/150 @hanm I believe we do have the same issue with the C client, I don't see it re-resolving addresses there, I need to have a closer look, though. --- 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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98385538
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -87,15 +75,104 @@ public
StaticHostProvider(Collection serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if(addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!connectedSinceNext) {
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+if
(!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+LOG.info("Resolving again hostname: {}",
getHostString(curAddr));
+try {
+int thePort = curAddr.getPort();
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(curAddr));
+nextAdded = 0;
+nextRemoved = 0;
+if (resolvedAddresses.length == 1) {
+serverAddresses.set(currentIndex, new
InetSocketAddress(resolvedAddresses[0], thePort));
+nextAdded = nextRemoved = 1;
+LOG.debug("Newly resolved address: {}",
resolvedAddresses[0]);
+} else {
+int i = 0;
--- End diff --
Although this is more Java-like, it requires the creation of an additional
ArrayList, which is less efficient than creating a int counter. Unless there is
something wrong with the current code, I'd rather leave as is.
---
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 #157: ZOOKEEPER-2678: Discovery and Sync can take a very lon...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/157 Thanks for the patch @revans2 . It makes sense to port this change to both 3.5 and master. --- 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 #157: ZOOKEEPER-2678: Discovery and Sync can take a v...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/157#discussion_r98337403 --- Diff: src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java --- @@ -839,6 +839,13 @@ public void converseWithFollower(InputArchive ia, OutputArchive oa, Assert.assertEquals(1, f.self.getAcceptedEpoch()); Assert.assertEquals(1, f.self.getCurrentEpoch()); +//Wait for the edits to be written out --- End diff -- I need to think some more whether it makes any sense to add test cases for this. The test cases we already have probably cover this enough given that there is no real change of behavior. This change here is necessary, though. We don't really care about time in general in our tests because we can never be sure of the timing we will get across runs and with different settings. --- 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 #157: ZOOKEEPER-2678: Discovery and Sync can take a v...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/157#discussion_r98336253
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Learner.java ---
@@ -364,10 +367,12 @@ else if (qp.getType() == Leader.SNAP) {
long lastQueued = 0;
-// in V1.0 we take a snapshot when we get the NEWLEADER
message, but in pre V1.0
+// in Zab V1.0 (ZK 3.4+) we might take a snapshot when we get
the NEWLEADER message, but in pre V1.0
// we take the snapshot at the UPDATE, since V1.0 also gets
the UPDATE (after the NEWLEADER)
// we need to make sure that we don't take the snapshot twice.
-boolean snapshotTaken = false;
+boolean isPreZAB1_0 = true;
+//If we are not going to take the snapshot be sure the edits
are not applied in memory
+boolean writeToEditLog = !snapshotNeeded;
--- End diff --
The changes here are using `edit` to refer to `txns`. I'd rather use `txn`
to be consistent across the project. Specifically here, you're using `EditLog`
to refer to the `TxnLog`, please change accordingly to have it consistent
across the project.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97446760
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection
serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+/**
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * It evaluates to a hostname if one is available and otherwise it
returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString;
+InetAddress ia = addr.getAddress();
+
+if (ia != null) {
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!connectedSinceNext) {
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+if
(!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+try {
+int thePort = curAddr.getPort();
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(curAddr));
+if (resolvedAddresses.length == 1) {
+serverAddresses.set(currentIndex, new
InetSocketAddress(resolvedAddresses[0], thePort));
+} else {
+serverAddresses.remove(currentIndex);
--- End diff --
@afine check the new changes to see if they address this and make sense.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97437693
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -87,15 +86,69 @@ public StaticHostProvider(Collection
serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+/**
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * It evaluates to a hostname if one is available and otherwise it
returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString;
+InetAddress ia = addr.getAddress();
+
+if (ia != null) {
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!connectedSinceNext) {
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+if
(!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+try {
+int thePort = curAddr.getPort();
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(curAddr));
+if (resolvedAddresses.length == 1) {
+serverAddresses.set(currentIndex, new
InetSocketAddress(resolvedAddresses[0], thePort));
+} else {
+serverAddresses.remove(currentIndex);
+for (InetAddress resolvedAddress :
resolvedAddresses) {
+InetSocketAddress newAddr = new
InetSocketAddress(resolvedAddress, thePort);
+if (!serverAddresses.contains(newAddr)) {
+serverAddresses.add(newAddr);
+}
+}
--- End diff --
We shuffle initially to avoid having all clients connecting to the same
server in the case they are all given the same connect string. If the array of
addresses has already been shuffled (in the constructor), then the order
followed in this method will be the shuffled one. I don't see a strong reason
for re-shuffling, as we are not bringing it back to the original order by
resolving again.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97436370
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection
serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+/**
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * It evaluates to a hostname if one is available and otherwise it
returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
--- End diff --
OK.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97436195
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection
serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
-address.getHostName());
+InetAddress resolvedAddresses[];
+try {
+Method m =
InetSocketAddress.class.getDeclaredMethod("getHostString");
+m.setAccessible(true);
+resolvedAddresses = InetAddress.getAllByName((String)
m.invoke(address));
+} catch (IllegalAccessException e) {
+resolvedAddresses =
InetAddress.getAllByName(getHostString(address));
+} catch (NoSuchMethodException e) {
+resolvedAddresses =
InetAddress.getAllByName(getHostString(address));
+} catch (InvocationTargetException e) {
+resolvedAddresses =
InetAddress.getAllByName(getHostString(address));
+}
--- End diff --
That's possibly another issue with this way of exposing `getHostString, the
presence of a security manager could prevent us from doing it as expected.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97435427
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection
serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
-address.getHostName());
+InetAddress resolvedAddresses[];
+try {
--- End diff --
I'm still not sure we should do this. I'm concerned about making that
method visible while the original intention was not to expose it. Are you aware
of any other project that has done this for `getHostString`?
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96642443
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection
serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
-address.getHostName());
+InetAddress resolvedAddresses[];
+try {
--- End diff --
@hanm have a look at this, please.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96442215
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection
serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+/**
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * It evaluates to a hostname if one is available and otherwise it
returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString;
+InetAddress ia = addr.getAddress();
+
+if (ia != null) {
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!connectedSinceNext) {
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+if
(!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+try {
+int thePort = curAddr.getPort();
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(curAddr));
+if (resolvedAddresses.length == 1) {
+serverAddresses.set(currentIndex, new
InetSocketAddress(resolvedAddresses[0], thePort));
+} else {
+serverAddresses.remove(currentIndex);
+for (InetAddress resolvedAddress :
resolvedAddresses) {
+InetSocketAddress newAddr = new
InetSocketAddress(resolvedAddress, thePort);
+if (!serverAddresses.contains(newAddr)) {
+serverAddresses.add(newAddr);
+}
+}
+}
+} catch (UnknownHostException e) {
+LOG.warn("Cannot re-resolve server: " + curAddr + "
UnknownHostException: " + e);
--- End diff --
If the `StaticHostProvider` constructor didn't throw an
`UnknownHostException`, then I'd think that all names and addresses we have are
good. I'm not sure what could cause an `UnknownHostException` in `next()` other
than some transient error. If that's right, then I'm not sure we should be
adding or removing anything.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96439377
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection
serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+/**
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * It evaluates to a hostname if one is available and otherwise it
returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
--- End diff --
We can do it, but I'm not super convinced we should because we are
essentially using a method with undocumented API. Perhaps it does the same as
the one in Java 7, with the difference that it is public, but I'm worried that
there could be some correctness issue involved. Do you know more about it?
In any case, I'm going to push the changes so that we see how it looks like.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96433850
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection
serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+/**
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * It evaluates to a hostname if one is available and otherwise it
returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString;
+InetAddress ia = addr.getAddress();
+
+if (ia != null) {
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!connectedSinceNext) {
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+if
(!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+try {
+int thePort = curAddr.getPort();
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(curAddr));
+if (resolvedAddresses.length == 1) {
+serverAddresses.set(currentIndex, new
InetSocketAddress(resolvedAddresses[0], thePort));
+} else {
+serverAddresses.remove(currentIndex);
+for (InetAddress resolvedAddress :
resolvedAddresses) {
+InetSocketAddress newAddr = new
InetSocketAddress(resolvedAddress, thePort);
+if (!serverAddresses.contains(newAddr)) {
+serverAddresses.add(newAddr);
+}
+}
+}
+} catch (UnknownHostException e) {
+LOG.warn("Cannot re-resolve server: " + curAddr + "
UnknownHostException: " + e);
--- End diff --
I don't think I can do both, see the API docs of slf4j:
https://www.slf4j.org/api/org/slf4j/Logger.html
In the case you are, tell me which one is your favorite. I'd say the curly
braces.
---
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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/150#discussion_r96431605 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -87,12 +73,69 @@ public StaticHostProvider(Collection serverAddresses) Collections.shuffle(this.serverAddresses); } +/** + * In Java 7, we have a method getHostString, but earlier versions do not support it. + * This method is to provide a replacement for InetSocketAddress.getHostString(). + * + * It evaluates to a hostname if one is available and otherwise it returns the --- End diff -- I've added a phrase to the return tag. --- 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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/122 @hanm @Randgalt sounds like a plan, let's follow the steps that Michael lined up above. --- 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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/122 @Randgalt >> when are we going to be removing these deprecated methods, in trunk > Maybe when we get a stable release of 3.5? Are you OK with us removing the deprecated methods when 3.5 becomes GA? > Agree, for trunk the change would be just rename ZooKeeperAdmin::reconfig to ZooKeeperAdmin::reconfigure so it's consistent with branch-3.5 (with some documentation updates and tests update.). We need to either create a new issue or have a different pull request for 3.5 under this same issue. I'd rather do the latter just so that we wrap this up in one shot, but it depends on whether @Randgalt is willing to do 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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/150#discussion_r96125997 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -45,6 +45,9 @@ private int currentIndex = -1; +// Don't re-resolve on first next() call +private boolean connectedSinceNext = true; --- End diff -- All calls to `next` and `onConnected` are from the `sendThread`. I don't see a reason for making volatile, unless we are doing it defensively. Let me know if I'm missing anything. Note that this pull request is for the 3.4 branch, we need a different patch for 3.5 and master. --- 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 #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/150 The error is expected because we haven't setup QA to build out of the 3.4 branch. --- 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 #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
GitHub user fpj opened a pull request: https://github.com/apache/zookeeper/pull/150 ZOOKEEPER-2184: Zookeeper Client should re-resolve hosts when connection attempts fail This is a version of the patch for ZK-2184 for the 3.4 branch, compatible with Java 6. You can merge this pull request into a Git repository by running: $ git pull https://github.com/fpj/zookeeper ZK-2184 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/150.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 #150 commit fbaa47e3a96f166cfae45070b0724780f13714e9 Author: fpj Date: 2017-01-14T16:58:15Z ZOOKEEPER-2184: Zookeeper Client should re-resolve hosts when connection attempts fail --- 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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() methods that...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/122 Just so that I understand, when are we going to be removing this methods, in trunk? I'm asking this for two reasons: 1. So that users know in which version these methods are going away 2. What changes we need to apply to trunk. In trunk, we would need to at least change in the admin class `reconfig()` to `reconfigure()`, but it sounds like we don't need to bring back the old `reconfig` methods. --- 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 #122: [ZOOKEEPER-2642] Resurrect the reconfig() metho...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/122#discussion_r95591971 --- Diff: src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml --- @@ -300,6 +300,11 @@ server.3=125.23.63.25:2782:2785:participant from ZooKeeper class, and use of this API requires ACL setup and user authentication (see for more information.). + +Note: for temporary backward compatibility, the reconfig() APIs will remain in ZooKeeper.java + where they were for a few alpha versions of 3.5.x. However, these APIs are deprecated and users + should move to the reconfig() APIs in ZooKeeperAdmin.java. --- End diff -- Small typo: `reconfig()` should be `reconfigure()`. --- 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 #128: ZOOKEEPER-2647: Fix TestReconfigServer.cc
GitHub user fpj opened a pull request: https://github.com/apache/zookeeper/pull/128 ZOOKEEPER-2647: Fix TestReconfigServer.cc You can merge this pull request into a Git repository by running: $ git pull https://github.com/fpj/zookeeper ZK-2647 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/128.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 #128 commit 3f4934a5a0be6b75410b1902c08ecfa4130e07f5 Author: fpj Date: 2016-12-16T11:40:54Z ZOOKEEPER-2647: Fix TestReconfigServer.cc --- 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 #101: ZOOKEEPER-2383
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/101 This is including the commit I just did for ZOOKEEPER-761, could you rebase your branch, please, @rakeshadr ? --- 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 #127: ZOOKEEPER-2646: Java target in branch 3.4 doesn't matc...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/127 The test failure is simply because the pr test script doesn't exist in the 3.4 branch. --- 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 #127: ZOOKEEPER-2646: Java target in branch 3.4 doesn...
GitHub user fpj opened a pull request: https://github.com/apache/zookeeper/pull/127 ZOOKEEPER-2646: Java target in branch 3.4 doesn't match documentation You can merge this pull request into a Git repository by running: $ git pull https://github.com/fpj/zookeeper ZK-2646 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/127.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 #127 commit e6732f17d8df8b4815e4464f972f56ddc2322aea Author: fpj Date: 2016-12-15T22:26:36Z ZOOKEEPER-2646: Java target in branch 3.4 doesn't match documentation --- 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 #101: ZOOKEEPER-2383
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/101#discussion_r92712695
--- Diff:
src/java/test/org/apache/zookeeper/server/ZooKeeperServerStartupTest.java ---
@@ -0,0 +1,266 @@
+/**
+ * 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.server;
+
+import static
org.apache.zookeeper.client.FourLetterWordMain.send4LetterWord;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+import org.apache.zookeeper.test.ClientBase;
+import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This class tests the startup behavior of ZooKeeper server.
+ */
+public class ZooKeeperServerStartupTest extends ZKTestCase {
+private static final Logger LOG = LoggerFactory
+.getLogger(ZooKeeperServerStartupTest.class);
+private static int PORT = PortAssignment.unique();
+private static String HOST = "127.0.0.1";
+private static String HOSTPORT = HOST + ":" + PORT;
+private static final String ZK_NOT_SERVING = "This ZooKeeper instance
is not currently serving requests";
+
+private ServerCnxnFactory servcnxnf;
+private ZooKeeperServer zks;
+private File tmpDir;
+private CountDownLatch startupDelayLatch = new CountDownLatch(1);
+
+@After
+public void teardown() throws Exception {
+// count down to avoid infinite blocking call due to this latch, if
+// any.
+startupDelayLatch.countDown();
+
+if (servcnxnf != null) {
+servcnxnf.shutdown();
+}
+if (zks != null) {
+zks.shutdown();
+}
+if (zks.getZKDatabase() != null) {
+zks.getZKDatabase().close();
+}
+ClientBase.recursiveDelete(tmpDir);
+}
+
+/**
+ * Test case for
+ * {@link https://issues.apache.org/jira/browse/ZOOKEEPER-2383}.
+ */
+@Test(timeout = 3)
+public void testClientConnectionRequestDuringStartupWithNIOServerCnxn()
+throws Exception {
+tmpDir = ClientBase.createTmpDir();
+ClientBase.setupTestEnv();
+
+startSimpleZKServer(startupDelayLatch);
+SimpleZooKeeperServer simplezks = (SimpleZooKeeperServer) zks;
+Assert.assertTrue(
+"Failed to invoke zks#startup() method during server
startup",
+simplezks.waitForStartupInvocation(10));
+
+CountdownWatcher watcher = new CountdownWatcher();
+ZooKeeper zkClient = new ZooKeeper(HOSTPORT,
+ClientBase.CONNECTION_TIMEOUT, watcher);
+
+Assert.assertFalse(
+"Since server is not fully started, zks#createSession()
shouldn't be invoked",
+simplezks.waitForSessionCreation(5));
+
+LOG.info(
+"Decrements the count of the latch, so that server will
proceed with startup");
+startupDelayLatch.countDown();
+
+Assert.assertTrue("waiting for server being up ", ClientBase
+.waitForServerUp(HOSTPORT, ClientBase.CONNECTION_TIMEOUT));
+
+Assert.assertTrue(
+"Failed to invoke zks#createSession() method during client
session creation",
+simplezks.waitForSessionCreation(5));
+watcher.waitForConnected(Clie
[GitHub] zookeeper issue #90: ZOOKEEPER-761: Remove *synchronous* calls from the *sin...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/90 It sounds like there is a conflict in `TestReconfigServer`, could you resolve it, please @breed ? --- 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 #109: Zookeeper 2542
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/109 agreed, @rakeshadr 's branch seems to be out of sync. --- 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 #102: ZOOKEEPER-2628: Fix findbug warnings.
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/102 @hanm I've left a few comments, but in general looks pretty good. I noticed that there are also unaddressed comments from @lvfangmin and @eribeiro . Could you take care of those so that we can check this in, please? --- 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.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/102#discussion_r89264869
--- Diff: src/java/main/org/apache/jute/compiler/JRecord.java ---
@@ -403,168 +410,165 @@ public void genJavaCode(File outputDirectory)
throws IOException {
} else if (!pkgdir.isDirectory()) {
throw new IOException(pkgpath + " is not a directory.");
}
-File jfile = new File(pkgdir, getName()+".java");
-FileWriter jj = new FileWriter(jfile);
-jj.write("// File generated by hadoop record compiler. Do not
edit.\n");
-jj.write("/**\n");
-jj.write("* Licensed to the Apache Software Foundation (ASF) under
one\n");
-jj.write("* or more contributor license agreements. See the
NOTICE file\n");
-jj.write("* distributed with this work for additional
information\n");
-jj.write("* regarding copyright ownership. The ASF licenses this
file\n");
-jj.write("* to you under the Apache License, Version 2.0 (the\n");
-jj.write("* \"License\"); you may not use this file except in
compliance\n");
-jj.write("* with the License. You may obtain a copy of the
License at\n");
-jj.write("*\n");
-jj.write("* http://www.apache.org/licenses/LICENSE-2.0\n";);
-jj.write("*\n");
-jj.write("* Unless required by applicable law or agreed to in
writing, software\n");
-jj.write("* distributed under the License is distributed on an
\"AS IS\" BASIS,\n");
-jj.write("* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express or implied.\n");
-jj.write("* See the License for the specific language governing
permissions and\n");
-jj.write("* limitations under the License.\n");
-jj.write("*/\n");
-jj.write("\n");
-jj.write("package "+getJavaPackage()+";\n\n");
-jj.write("import org.apache.jute.*;\n");
-jj.write("public class "+getName()+" implements Record {\n");
-for (Iterator i = mFields.iterator(); i.hasNext();) {
-JField jf = i.next();
-jj.write(jf.genJavaDecl());
-}
-jj.write(" public "+getName()+"() {\n");
-jj.write(" }\n");
+try (FileWriter jj = new FileWriter(new File(pkgdir,
getName()+".java"))) {
+jj.write("// File generated by hadoop record compiler. Do not
edit.\n");
+jj.write("/**\n");
+jj.write("* Licensed to the Apache Software Foundation (ASF)
under one\n");
+jj.write("* or more contributor license agreements. See the
NOTICE file\n");
+jj.write("* distributed with this work for additional
information\n");
+jj.write("* regarding copyright ownership. The ASF licenses
this file\n");
+jj.write("* to you under the Apache License, Version 2.0
(the\n");
+jj.write("* \"License\"); you may not use this file except in
compliance\n");
+jj.write("* with the License. You may obtain a copy of the
License at\n");
+jj.write("*\n");
+jj.write("* http://www.apache.org/licenses/LICENSE-2.0\n";);
+jj.write("*\n");
+jj.write("* Unless required by applicable law or agreed to in
writing, software\n");
+jj.write("* distributed under the License is distributed on an
\"AS IS\" BASIS,\n");
+jj.write("* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
either express or implied.\n");
+jj.write("* See the License for the specific language
governing permissions and\n");
+jj.write("* limitations under the License.\n");
+jj.write("*/\n");
+jj.write("\n");
+jj.write("package " + getJavaPackage() + ";\n\n");
+jj.write("import org.apache.jute.*;\n");
+jj.write("public class " + getName() + " implements Record
{\n");
+for (Iterator i = mFields.iterator(); i.hasNext(); ) {
+JField jf = i.next();
+jj.write(jf.genJavaDecl());
+}
+jj.write(" public " + getName() +
[GitHub] zookeeper pull request #102: ZOOKEEPER-2628: Fix findbug warnings.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/102#discussion_r89264584
--- Diff: src/java/main/org/apache/jute/compiler/JRecord.java ---
@@ -403,168 +410,165 @@ public void genJavaCode(File outputDirectory)
throws IOException {
} else if (!pkgdir.isDirectory()) {
throw new IOException(pkgpath + " is not a directory.");
}
-File jfile = new File(pkgdir, getName()+".java");
-FileWriter jj = new FileWriter(jfile);
-jj.write("// File generated by hadoop record compiler. Do not
edit.\n");
-jj.write("/**\n");
-jj.write("* Licensed to the Apache Software Foundation (ASF) under
one\n");
-jj.write("* or more contributor license agreements. See the
NOTICE file\n");
-jj.write("* distributed with this work for additional
information\n");
-jj.write("* regarding copyright ownership. The ASF licenses this
file\n");
-jj.write("* to you under the Apache License, Version 2.0 (the\n");
-jj.write("* \"License\"); you may not use this file except in
compliance\n");
-jj.write("* with the License. You may obtain a copy of the
License at\n");
-jj.write("*\n");
-jj.write("* http://www.apache.org/licenses/LICENSE-2.0\n";);
-jj.write("*\n");
-jj.write("* Unless required by applicable law or agreed to in
writing, software\n");
-jj.write("* distributed under the License is distributed on an
\"AS IS\" BASIS,\n");
-jj.write("* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express or implied.\n");
-jj.write("* See the License for the specific language governing
permissions and\n");
-jj.write("* limitations under the License.\n");
-jj.write("*/\n");
-jj.write("\n");
-jj.write("package "+getJavaPackage()+";\n\n");
-jj.write("import org.apache.jute.*;\n");
-jj.write("public class "+getName()+" implements Record {\n");
-for (Iterator i = mFields.iterator(); i.hasNext();) {
-JField jf = i.next();
-jj.write(jf.genJavaDecl());
-}
-jj.write(" public "+getName()+"() {\n");
-jj.write(" }\n");
+try (FileWriter jj = new FileWriter(new File(pkgdir,
getName()+".java"))) {
+jj.write("// File generated by hadoop record compiler. Do not
edit.\n");
+jj.write("/**\n");
+jj.write("* Licensed to the Apache Software Foundation (ASF)
under one\n");
+jj.write("* or more contributor license agreements. See the
NOTICE file\n");
+jj.write("* distributed with this work for additional
information\n");
+jj.write("* regarding copyright ownership. The ASF licenses
this file\n");
+jj.write("* to you under the Apache License, Version 2.0
(the\n");
+jj.write("* \"License\"); you may not use this file except in
compliance\n");
+jj.write("* with the License. You may obtain a copy of the
License at\n");
+jj.write("*\n");
+jj.write("* http://www.apache.org/licenses/LICENSE-2.0\n";);
+jj.write("*\n");
+jj.write("* Unless required by applicable law or agreed to in
writing, software\n");
+jj.write("* distributed under the License is distributed on an
\"AS IS\" BASIS,\n");
+jj.write("* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
either express or implied.\n");
+jj.write("* See the License for the specific language
governing permissions and\n");
+jj.write("* limitations under the License.\n");
+jj.write("*/\n");
+jj.write("\n");
+jj.write("package " + getJavaPackage() + ";\n\n");
+jj.write("import org.apache.jute.*;\n");
+jj.write("public class " + getName() + " implements Record
{\n");
+for (Iterator i = mFields.iterator(); i.hasNext(); ) {
+JField jf = i.next();
+jj.write(jf.genJavaDecl());
+}
+jj.write(" public " + getName() +
[GitHub] zookeeper pull request #102: ZOOKEEPER-2628: Fix findbug warnings.
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/102#discussion_r89283922 --- Diff: src/java/main/org/apache/zookeeper/server/command/FourLetterCommands.java --- @@ -151,9 +153,13 @@ */ public final static int telnetCloseCmd = 0xfff4fffd; -public final static HashMap cmd2String = +final static HashMap cmd2String = --- End diff -- If we can remove this `public`, then I think we should. Also agree with the consistent `Map` declaration 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 #102: ZOOKEEPER-2628: Fix findbug warnings.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/102#discussion_r89283699
--- Diff: src/java/main/org/apache/zookeeper/cli/DeleteCommand.java ---
@@ -56,14 +56,7 @@ public CliCommand parse(String[] cmdArgs) throws
CliParseException {
}
private void retainCompatibility(String[] cmdArgs) throws
CliParseException {
-// delete path [version]
if (args.length > 2) {
-// rewrite to option
-String [] newCmd = new String[4];
--- End diff --
Why are we removing this rewrite?
---
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.
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/102#discussion_r89284431 --- Diff: src/java/test/config/findbugsExcludeFile.xml --- @@ -144,4 +144,10 @@ +
[GitHub] zookeeper pull request #102: ZOOKEEPER-2628: Fix findbug warnings.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/102#discussion_r89285632
--- Diff: src/java/main/org/apache/jute/compiler/JRecord.java ---
@@ -576,174 +580,174 @@ public void genCsharpCode(File outputDirectory)
throws IOException {
} else if (!outputDirectory.isDirectory()) {
throw new IOException(outputDirectory + " is not a
directory.");
}
-File csharpFile = new File(outputDirectory, getName()+".cs");
-FileWriter cs = new FileWriter(csharpFile);
-cs.write("// File generated by hadoop record compiler. Do not
edit.\n");
-cs.write("/**\n");
-cs.write("* Licensed to the Apache Software Foundation (ASF) under
one\n");
-cs.write("* or more contributor license agreements. See the
NOTICE file\n");
-cs.write("* distributed with this work for additional
information\n");
-cs.write("* regarding copyright ownership. The ASF licenses this
file\n");
-cs.write("* to you under the Apache License, Version 2.0 (the\n");
-cs.write("* \"License\"); you may not use this file except in
compliance\n");
-cs.write("* with the License. You may obtain a copy of the
License at\n");
-cs.write("*\n");
-cs.write("* http://www.apache.org/licenses/LICENSE-2.0\n";);
-cs.write("*\n");
-cs.write("* Unless required by applicable law or agreed to in
writing, software\n");
-cs.write("* distributed under the License is distributed on an
\"AS IS\" BASIS,\n");
-cs.write("* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express or implied.\n");
-cs.write("* See the License for the specific language governing
permissions and\n");
-cs.write("* limitations under the License.\n");
-cs.write("*/\n");
-cs.write("\n");
-cs.write("using System;\n");
-cs.write("using Org.Apache.Jute;\n");
-cs.write("\n");
-cs.write("namespace "+getCsharpNameSpace()+"\n");
-cs.write("{\n");
-
-String className = getCsharpName();
-cs.write("public class "+className+" : IRecord, IComparable \n");
-cs.write("{\n");
-cs.write(" public "+ className +"() {\n");
-cs.write(" }\n");
-
-cs.write(" public "+className+"(\n");
-int fIdx = 0;
-int fLen = mFields.size();
-for (Iterator i = mFields.iterator(); i.hasNext(); fIdx++)
{
-JField jf = i.next();
-cs.write(jf.genCsharpConstructorParam(jf.getCsharpName()));
-cs.write((fLen-1 == fIdx)?"":",\n");
-}
-cs.write(") {\n");
-fIdx = 0;
-for (Iterator i = mFields.iterator(); i.hasNext(); fIdx++)
{
-JField jf = i.next();
-cs.write(jf.genCsharpConstructorSet(jf.getCsharpName()));
-}
-cs.write(" }\n");
-fIdx = 0;
-for (Iterator i = mFields.iterator(); i.hasNext(); fIdx++)
{
-JField jf = i.next();
-cs.write(jf.genCsharpGetSet(fIdx));
+
+try (FileWriter cs = new FileWriter(new File(outputDirectory,
getName() + ".cs"));) {
+cs.write("// File generated by hadoop record compiler. Do not
edit.\n");
+cs.write("/**\n");
+cs.write("* Licensed to the Apache Software Foundation (ASF)
under one\n");
+cs.write("* or more contributor license agreements. See the
NOTICE file\n");
+cs.write("* distributed with this work for additional
information\n");
+cs.write("* regarding copyright ownership. The ASF licenses
this file\n");
+cs.write("* to you under the Apache License, Version 2.0
(the\n");
+cs.write("* \"License\"); you may not use this file except in
compliance\n");
+cs.write("* with the License. You may obtain a copy of the
License at\n");
+cs.write("*\n");
+cs.write("* http://www.apache.org/licenses/LICENSE-2.0\n";);
+cs.write("*\n");
+cs.write("* Unless required by applicable law or agreed to in
wr
[GitHub] zookeeper issue #73: practise git
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/73 @eribeiro I'm not sure I'm able to close it, is it only the creator who can close 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 issue #90: ZOOKEEPER-761: Remove *synchronous* calls from the *sin...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/90 It looks good, I just left a comment about creating a jira before we forget. Could you do it before we close this issue, please @breed ? --- 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object into auth...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/84 @Randgalt thanks for the recent changes, they look fine for me, except that we didn't get QA to run on it. @lvfangmin there are a few levels of indirection here, which doesn't make it look very pretty, but I don't see a much better way of doing it. how do you see the recent changes, do they look goo to you? --- 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r87957495 --- Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java --- @@ -543,7 +546,7 @@ protected void pRequest2Txn(int type, long zxid, Request request, } nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE); -checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo); +checkACL(zks, request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, "/", null); --- End diff -- `null` tells me that the method is not expecting a value from the caller in that position, which is better than an arbitrary value. I think it is fine to change it to `null`. --- 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r87957002
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml
---
@@ -1193,6 +1193,55 @@ authProvider.2=com.f.MyAuth2
only one will be used. Also all servers must have the same plugins
defined, otherwise clients using
the authentication schemes provided by the plugins will have problems
connecting to some servers.
+
+ Added in 3.6.0: An alternate
abstraction is available for pluggable
+authentication. It provides additional arguments.
+
+
+
+public abstract class ServerAuthenticationProvider implements
AuthenticationProvider {
--- End diff --
Ok.
---
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 #98: ZOOKEEPER-2479
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/98 +1, LGTM. @eribeiro is this ready according to you? --- 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 #104: ZOOKEEPER-2631: Make issue extraction in the gi...
GitHub user fpj opened a pull request: https://github.com/apache/zookeeper/pull/104 ZOOKEEPER-2631: Make issue extraction in the git pull request script more robust You can merge this pull request into a Git repository by running: $ git pull https://github.com/fpj/zookeeper ZK-2631 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/104.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 #104 commit a26d74f955144d541ee96bcf58bd80ff43c7a0d7 Author: fpj Date: 2016-11-11T14:22:17Z ZOOKEEPER-2631: Make issue extraction in the git pull request script more robust --- 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 fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/96#discussion_r87536973
--- 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 --
@hanm hmm, I'm not sure about this. In the changes for `DataTree`, we only
set the ACL of the `/zookeeper/config` znode, but setting ACLs was something we
were doing before, so I'm confused about why we can have a mix of znode records
and ACL records with the changes proposed here. Could you clarify, please?
---
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 #73: practise git
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/73 @eribeiro yeah, this should be closed. --- 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 fpj commented on the issue: https://github.com/apache/zookeeper/pull/97 The failures are the same as before: `@author` string in the script and new findbugs warnings due to jenkins upgrade. --- 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 fpj commented on the issue: https://github.com/apache/zookeeper/pull/97 @breed @rgs1 let's get this in, I'll create a jira to cleanup the script. Note that a lot of the issues you pointed out are legacy from the other script, so we will probably want to address in both. --- 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 fpj commented on the issue: https://github.com/apache/zookeeper/pull/97 @hanm The `DEVELOPER` mode is for running locally, so initially I left it mostly unchanged and the original script takes a patch file to test. Because of your feedback, I thought that it might be better to take a pull request URL instead to test, so I changed the script for the developer mode a bit to do 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 issue #97: ZOOKEEPER-2624: Add test script for pull requests
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/97 The failure is actually expected because the script contains the `@author` string. --- 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 fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/97#discussion_r86386241
--- Diff: src/java/test/bin/test-github-pr.sh ---
@@ -0,0 +1,607 @@
+#!/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
+
+ ;;
+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_FILE contains the location of the patchfile
+ PATCH_FILE=$2
+ if [[ ! -e "$PATCH_FILE" ]] ; then
+echo "Unable to locate the patch file $PATCH_FILE"
+cleanupAndExit 0
+ fi
+ PATCH_DIR=$3
+ ### 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
+ defect=`basename $PATCH_FILE`
--- End diff --
not really, we care about it to write the report to the jira.
---
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 fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/97#discussion_r86386008 --- Diff: build.xml --- @@ -1622,6 +1623,26 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + + + + + + + + + + + + + + + + + --- End diff -- yes, it was intentionally left out. the `defect` number is the jira number. in the jira qa, that is being set by the trigger of the jira patch. if you look further down in the script, you'll see that I'm setting the `defect` variable by extracting the number from the PR title. I expect the PR title to contain the jira title `ZOOKEEPER-: Bla bla bla`. --- 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object into auth...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/84 @Randgalt I'm sorry that QA isn't working for pull requests yet, could please create a diff patch and upload to the jira so that we can get QA to run on 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 fpj opened a pull request: https://github.com/apache/zookeeper/pull/97 ZOOKEEPER-2624: Add test script for pull requests You can merge this pull request into a Git repository by running: $ git pull https://github.com/fpj/zookeeper ZK-2624 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/97.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 #97 commit 1929614a7ef076d9e2bd1191b3341207f2e4411e Author: fpj Date: 2016-10-31T12:13:59Z Initial cut of the script and changes to build.xml --- 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 #94: ZOOKEEPER-2014: Only admin should be allowed to reconfi...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/94 @hanm since there is no comment, would you mind closing this PR and resubmitting it to see if QA picks it up? --- 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r85642051
--- Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java ---
@@ -0,0 +1,131 @@
+/**
+ * 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 org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.List;
+
+public class KeyAuthClientTest extends ClientBase {
+private static final Logger LOG =
LoggerFactory.getLogger(KeyAuthClientTest.class);
+static {
+
System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider");
+}
+
+public void createNodePrintAcl(ZooKeeper zk, String path, String
testName) {
+ try {
+LOG.debug("KeyAuthenticationProvider Creating Test
Node:"+path+".\n");
+zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
+List acls = zk.getACL(path, null);
+LOG.debug("Node: "+path+" Test:"+testName+" ACLs:");
+for (ACL acl : acls) {
+ LOG.debug(" "+acl.toString());
--- End diff --
Same here, space around symbols, please.
---
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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r85642202
--- Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java ---
@@ -0,0 +1,131 @@
+/**
+ * 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 org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.List;
+
+public class KeyAuthClientTest extends ClientBase {
+private static final Logger LOG =
LoggerFactory.getLogger(KeyAuthClientTest.class);
+static {
+
System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider");
+}
+
+public void createNodePrintAcl(ZooKeeper zk, String path, String
testName) {
+ try {
+LOG.debug("KeyAuthenticationProvider Creating Test
Node:"+path+".\n");
+zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
+List acls = zk.getACL(path, null);
+LOG.debug("Node: "+path+" Test:"+testName+" ACLs:");
+for (ACL acl : acls) {
+ LOG.debug(" "+acl.toString());
+}
+ } catch (Exception e) {
+ LOG.debug(" EXCEPTION THROWN", e);
+ }
+}
+
+public void testPreAuth() throws Exception {
+ZooKeeper zk = createClient();
+zk.addAuthInfo("key", "25".getBytes());
+try {
+createNodePrintAcl(zk, "/pre", "testPreAuth");
+zk.setACL("/", Ids.CREATOR_ALL_ACL, -1);
+zk.getChildren("/", false);
+zk.create("/abc", null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
+zk.setData("/abc", "testData1".getBytes(), -1);
+zk.create("/key", null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
+zk.setData("/key", "5".getBytes(), -1);
+Thread.sleep(1000);
--- End diff --
I'm not sure why we need this `sleep` 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r85642282
--- Diff:
src/java/main/org/apache/zookeeper/server/auth/ServerAuthenticationProvider.java
---
@@ -0,0 +1,79 @@
+/**
+ * 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.server.auth;
+
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.server.ServerCnxn;
+import org.apache.zookeeper.server.ZooKeeperServer;
+
+import java.util.List;
+
+/**
+ * A variation on {@link AuthenticationProvider} that provides additional
+ * parameters for more detailed authentication
+ */
+public abstract class ServerAuthenticationProvider implements
AuthenticationProvider {
+/**
+ * This method is called when a client passes authentication data for
this
+ * scheme. The authData is directly from the authentication packet. The
+ * implementor may attach new ids to the authInfo field of cnxn or may
use
+ * cnxn to send packets back to the client.
+ *
+ * @param cnxn
+ *the cnxn that received the authentication
information.
+ * @param authData
+ *the authentication data received.
+ * @return indication of success or failure
+ */
+public abstract KeeperException.Code
handleAuthentication(ZooKeeperServer zks, ServerCnxn cnxn, byte authData[]);
+
+/**
+ * This method is called to see if the given id matches the given id
+ * expression in the ACL. This allows schemes to use application
specific
+ * wild cards.
+ *
+ * @param zks
+ *the ZooKeeper server instance
+ * @param cnxn
+ *the active server connection being authenticated
+ * @param path
+ *the path of the operation being authenticated
+ * @param id
+ *the id to check.
+ * @param aclExpr
+ *the expression to match ids against.
+ * @param perm
+ *the permission value being authenticated
+ * @param setAcls
+ *for set ACL operations, the list of ACLs being set.
Otherwise null.
+ * @return true if the arguments can be matched by the expression.
+ */
+public abstract boolean matches(ZooKeeperServer zks, ServerCnxn cnxn,
String path, String id, String aclExpr, int perm, List setAcls);
--- End diff --
Can we break this line, please?
---
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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object into auth...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/84 @Randgalt My comments are pretty minor, but it will give it a chance to QA to test this PR. --- 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r85642270
--- Diff:
src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java ---
@@ -31,7 +31,15 @@
private static boolean initialized = false;
private static HashMap
authenticationProviders =
-new HashMap();
+new HashMap<>();
+
+//VisibleForTesting
+public static void reset() {
--- End diff --
Is this used anywhere?
---
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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r85641947
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml
---
@@ -1193,6 +1193,55 @@ authProvider.2=com.f.MyAuth2
only one will be used. Also all servers must have the same plugins
defined, otherwise clients using
the authentication schemes provided by the plugins will have problems
connecting to some servers.
+
+ Added in 3.6.0: An alternate
abstraction is available for pluggable
+authentication. It provides additional arguments.
+
+
+
+public abstract class ServerAuthenticationProvider implements
AuthenticationProvider {
--- End diff --
Indentation? Not sure if it matters within the listing block.
---
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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r85642186
--- Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java ---
@@ -0,0 +1,131 @@
+/**
+ * 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 org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.List;
+
+public class KeyAuthClientTest extends ClientBase {
+private static final Logger LOG =
LoggerFactory.getLogger(KeyAuthClientTest.class);
+static {
+
System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider");
+}
+
+public void createNodePrintAcl(ZooKeeper zk, String path, String
testName) {
+ try {
+LOG.debug("KeyAuthenticationProvider Creating Test
Node:"+path+".\n");
+zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
+List acls = zk.getACL(path, null);
+LOG.debug("Node: "+path+" Test:"+testName+" ACLs:");
+for (ACL acl : acls) {
+ LOG.debug(" "+acl.toString());
+}
+ } catch (Exception e) {
+ LOG.debug(" EXCEPTION THROWN", e);
+ }
+}
+
+public void testPreAuth() throws Exception {
+ZooKeeper zk = createClient();
+zk.addAuthInfo("key", "25".getBytes());
+try {
+createNodePrintAcl(zk, "/pre", "testPreAuth");
+zk.setACL("/", Ids.CREATOR_ALL_ACL, -1);
+zk.getChildren("/", false);
+zk.create("/abc", null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
+zk.setData("/abc", "testData1".getBytes(), -1);
+zk.create("/key", null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
+zk.setData("/key", "5".getBytes(), -1);
+Thread.sleep(1000);
+} catch (KeeperException e) {
+Assert.fail("test failed :" + e);
+}
+finally {
+zk.close();
+}
+}
+
+public void testMissingAuth() throws Exception {
+ZooKeeper zk = createClient();
+try {
+zk.getData("/abc", false, null);
+Assert.fail("Should not be able to get data");
+} catch (KeeperException correct) {
+// correct
+}
+try {
+zk.setData("/abc", "testData2".getBytes(), -1);
+Assert.fail("Should not be able to set data");
+} catch (KeeperException correct) {
+// correct
+} finally {
+zk.close();
+}
+}
+
+public void testValidAuth() throws Exception {
+ZooKeeper zk = createClient();
+// any multiple of 5 will do...
+zk.addAuthInfo("key", "25".getBytes());
+try {
+createNodePrintAcl(zk, "/valid", "testValidAuth");
+zk.getData("/abc", false, null);
+zk.setData("/abc", "testData3".getBytes(), -1);
+} catch (KeeperException.AuthFailedException e) {
+Assert.fail("test failed :" + e);
+} finally {
+zk.close();
+}
+}
+
+public void testValidA
[GitHub] zookeeper pull request #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85641968 --- Diff: src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java --- @@ -543,7 +546,7 @@ protected void pRequest2Txn(int type, long zxid, Request request, } nodeRecord = getRecordForPath(ZooDefs.CONFIG_NODE); -checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo); +checkACL(zks, request.cnxn, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo, "/", null); --- End diff -- Why is the path `"/"` 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/84#discussion_r85641975 --- Diff: src/java/main/org/apache/zookeeper/server/ServerCnxn.java --- @@ -51,7 +47,7 @@ final public static Object me = new Object(); private static final Logger LOG = LoggerFactory.getLogger(ServerCnxn.class); -protected ArrayList authInfo = new ArrayList(); +private Set authInfo = Collections.newSetFromMap(new ConcurrentHashMap()); --- End diff -- Ok. --- 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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r85642389
--- Diff:
src/java/main/org/apache/zookeeper/server/auth/WrappedAuthenticationProvider.java
---
@@ -0,0 +1,74 @@
+/**
+ * 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.server.auth;
+
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.ACL;
+import org.apache.zookeeper.server.ServerCnxn;
+import org.apache.zookeeper.server.ZooKeeperServer;
+
+import java.util.List;
+
+/**
+ * Provides backwards compatibility between older {@link
AuthenticationProvider}
+ * implementations and the new {@link ServerAuthenticationProvider}
interface.
+ */
+class WrappedAuthenticationProvider extends ServerAuthenticationProvider {
+private final AuthenticationProvider implementation;
+
+static ServerAuthenticationProvider wrap(AuthenticationProvider
provider) {
+return (provider instanceof ServerAuthenticationProvider) ?
(ServerAuthenticationProvider)provider
+: new WrappedAuthenticationProvider(provider);
+}
+
+private WrappedAuthenticationProvider(AuthenticationProvider
implementation) {
+this.implementation = implementation;
+}
+
+@Override
+public KeeperException.Code handleAuthentication(ZooKeeperServer zks,
ServerCnxn cnxn, byte[] authData) {
+return implementation.handleAuthentication(cnxn, authData);
+}
+
+/**
+ * {@inheritDoc}
--- End diff --
I've never used this before and from the name it implies that it will copy
the doc blurb from the super class when running the javadoc tool. Is this
correct? Do we have to do it for this method only or for others as well?
---
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 #84: [ZOOKEEPER-1525] Plumb ZooKeeperServer object in...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/84#discussion_r85642046
--- Diff: src/java/test/org/apache/zookeeper/test/KeyAuthClientTest.java ---
@@ -0,0 +1,131 @@
+/**
+ * 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 org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.ACL;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.List;
+
+public class KeyAuthClientTest extends ClientBase {
+private static final Logger LOG =
LoggerFactory.getLogger(KeyAuthClientTest.class);
+static {
+
System.setProperty("zookeeper.authProvider.1","org.apache.zookeeper.server.auth.KeyAuthenticationProvider");
+}
+
+public void createNodePrintAcl(ZooKeeper zk, String path, String
testName) {
+ try {
+LOG.debug("KeyAuthenticationProvider Creating Test
Node:"+path+".\n");
+zk.create(path, null, Ids.CREATOR_ALL_ACL, CreateMode.PERSISTENT);
+List acls = zk.getACL(path, null);
+LOG.debug("Node: "+path+" Test:"+testName+" ACLs:");
--- End diff --
space around symbols, please?
---
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 #95: ZOOKEEPER-2622: ZooTrace.logQuorumPacket does no...
GitHub user fpj opened a pull request: https://github.com/apache/zookeeper/pull/95 ZOOKEEPER-2622: ZooTrace.logQuorumPacket does nothing DO NOT MERGE You can merge this pull request into a Git repository by running: $ git pull https://github.com/fpj/zookeeper ZK-2622 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/95.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 #95 commit 76681a075e045107f149e5ab67756991faf1d8ba Author: fpj Date: 2016-10-28T09:14:51Z ZOOKEEPER-2622: ZooTrace.logQuorumPacket does nothing --- 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 #90: ZOOKEEPER-761: Remove *synchronous* calls from the *sin...
Github user fpj commented on the issue: https://github.com/apache/zookeeper/pull/90 @breed `make check` does not compile for me: ``` g++ -DHAVE_CONFIG_H -I. -I./include -I./tests -I./generated -DUSE_STATIC_LIB -DZKSERVER_CMD="\"./tests/zkServer.sh\"" -DZOO_IPV6_ENABLED -g -O2 -MT zktest_st-TestReconfigServer.o -MD -MP -MF .deps/zktest_st-TestReconfigServer.Tpo -c -o zktest_st-TestReconfigServer.o `test -f 'tests/TestReconfigServer.cc' || echo './'`tests/TestReconfigServer.cc In file included from /usr/include/cppunit/TestCase.h:6:0, from /usr/include/cppunit/TestCaller.h:5, from /usr/include/cppunit/extensions/HelperMacros.h:9, from tests/TestReconfigServer.cc:18: tests/TestReconfigServer.cc: In member function 'void TestReconfigServer::testRemoveFollower()': tests/TestReconfigServer.cc:153:73: error: 'zoo_getconfig' was not declared in this scope CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat)); ^ tests/TestReconfigServer.cc:174:32: error: 'zoo_reconfig' was not declared in this scope &stat); ^ In file included from /usr/include/cppunit/TestCase.h:6:0, from /usr/include/cppunit/TestCaller.h:5, from /usr/include/cppunit/extensions/HelperMacros.h:9, from tests/TestReconfigServer.cc:18: tests/TestReconfigServer.cc: In member function 'void TestReconfigServer::testNonIncremental()': tests/TestReconfigServer.cc:221:73: error: 'zoo_getconfig' was not declared in this scope CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat)); ^ tests/TestReconfigServer.cc:246:32: error: 'zoo_reconfig' was not declared in this scope &stat); ^ tests/TestReconfigServer.cc: In member function 'void TestReconfigServer::testRemoveConnectedFollower()': tests/TestReconfigServer.cc:313:72: error: 'zoo_reconfig' was not declared in this scope zoo_reconfig(zk, NULL, ss.str().c_str(), NULL, -1, buf, &len, &stat); ^ In file included from /usr/include/cppunit/TestCase.h:6:0, from /usr/include/cppunit/TestCaller.h:5, from /usr/include/cppunit/extensions/HelperMacros.h:9, from tests/TestReconfigServer.cc:18: tests/TestReconfigServer.cc:314:73: error: 'zoo_getconfig' was not declared in this scope CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_getconfig(zk, 0, buf, &len, &stat)); ``` have you tried running the C tests? --- 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. ---
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review151129
---
It is taking shape, thanks for the updates, Michael. I've left some more
comments when you have a minute.
src/c/tests/TestReconfigServer.cc (line 80)
<https://reviews.apache.org/r/51546/#comment219278>
We are setting up a cluster with a super password. Is it worth testing the
behavior for a cluster without the super digest set? I'm actually assuming that
we will warn the user and not let reconfig happen in the case the user forgets
to set the super password.
src/c/tests/TestReconfigServer.cc (line 342)
<https://reviews.apache.org/r/51546/#comment219275>
"... only admin / users that have permission can do reconfig."
src/c/tests/TestReconfigServer.cc (line 381)
<https://reviews.apache.org/r/51546/#comment219276>
Why do we need this sleep here?
src/c/tests/TestReconfigServer.cc (line 385)
<https://reviews.apache.org/r/51546/#comment219277>
and here?
src/c/tests/ZooKeeperQuorumServer.cc (line 29)
<https://reviews.apache.org/r/51546/#comment219279>
Minor: why is it configs (plural) rather than just config? it is just a
single configuration, yes?
src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (line 944)
<https://reviews.apache.org/r/51546/#comment219280>
This is great, here is a slightly modified version: _When the feature is
enabled, users can perform reconfigure operations through the ZooKeeper client
API or through the 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._
src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (line 951)
<https://reviews.apache.org/r/51546/#comment219281>
What happens if this is set inconsistently? If one server sets it and
another doesn't, then what is the expected behavior? Shall we document it?
src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 321)
<https://reviews.apache.org/r/51546/#comment219282>
"... the ability to change..." instead?
src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 322)
<https://reviews.apache.org/r/51546/#comment219283>
"It is thus possible for a malicious client to make arbitrary changes to an
ensemble, e.g., add a compromised server, remove legitimate servers." instead?
src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 331)
<https://reviews.apache.org/r/51546/#comment219284>
"... interact with a ZooKeeper ensemble..."
src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 338)
<https://reviews.apache.org/r/51546/#comment219285>
"... allow a user to configure different levels..."
Also, it is not really clear what are the different levels that this is
referring to. Could you clarify, please?
src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 353)
<https://reviews.apache.org/r/51546/#comment219286>
"... only the super user has ..."
src/java/main/org/apache/zookeeper/ClientCnxn.java (line 1526)
<https://reviews.apache.org/r/51546/#comment219287>
It is a bit odd that we are making this public. I suppose this is for
tests, so I'm wondering if we can extend the class in the tests to avoid making
the method public.
src/java/main/org/apache/zookeeper/ZooKeeperMain.java (line 88)
<https://reviews.apache.org/r/51546/#comment219290>
If `ZooKeeperAdmin` extends `ZooKeeper`, why do we need both a `ZooKeeper`
object and a `ZooKeeperAdmin` object here?
src/java/main/org/apache/zookeeper/ZooKeeperMain.java (line 281)
<https://reviews.apache.org/r/51546/#comment219288>
if we close the zk handle three lines above, then it is possible that
`zk.getState().isAlive()` is false, but zkAdmin is true, no?
I think this code should be something like:
if (zk.getState().isAlive()) {
if (zk != null) {
zk.close();
}
if (zkAdmin != null) {
zkAdmin.close();
}
}
src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 127)
<https://reviews.apache.org/r/51546/#comment219291>
Could you break the line, please?
src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 169)
<https://reviews.apache.org/r/51546/#comment219289>
White spaces.
src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 177)
<https://reviews.apache.org/r/51546/#comment219292>
Is access to ClientCnxn the main reason for extending `ZooKeepe
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51546/#review150301 --- Looks pretty good, Michael. I have left a few comments and questions when you have a minute. src/c/tests/TestReconfigServer.cc (line 78) <https://reviews.apache.org/r/51546/#comment218209> Is this saying that we don't need to test ACLs with reconfig for the C client? src/c/tests/TestReconfigServer.cc (line 80) <https://reviews.apache.org/r/51546/#comment218208> To confirm, setting skipACL to true isn't strictly necessary here, right? src/c/tests/ZooKeeperQuorumServer.cc (line 105) <https://reviews.apache.org/r/51546/#comment218210> The additional parameter adds optional parameters, so perhaps the name should reflect that it isn't a full config, but additional paramters instead. I'm wondering if instead of passing a string, which requires the caller to format it properly, we whould just pass the paramters explicitly. I don't feel strongly about it, but I wanted to run the idea by you. src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (line 942) <https://reviews.apache.org/r/51546/#comment218211> Should we actually explain what enabling/disabling reconfig entails? src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 152) <https://reviews.apache.org/r/51546/#comment218212> You may want to motivate why we would consider disabling the feature. Something along the lines of: "With reconfiguration enabled, we have a security concern that a malicious actor can make arbitrary changes to the configuration of a ZooKeeper ensemble, including adding a compromised server to the ensemble. We prefer to leave to the discretion of the user to decide whether to enable it or not and make sure that the appropriate security measure are in place." src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 314) <https://reviews.apache.org/r/51546/#comment218213> There is a bit of taste involved in this description. The need of security depends on lots of things and it is rarely black or white. I'd rather say like I suggested above that some users were concerned with the possibility of ensembles being arbitrarily reconfigured and we added the option of disabling it, so it is really up to the user to decide what he/she wants to do. src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 335) <https://reviews.apache.org/r/51546/#comment218214> This sentence is broken: "Clients that need to use reconfig commands or the reconfig API to change the state of a ZooKeeper ensemble should be configured as users that have write access to CONFIG_NODE." src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 337) <https://reviews.apache.org/r/51546/#comment218215> The contract is that by default only the super user can change the configuration, yes? src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java (line 19) <https://reviews.apache.org/r/51546/#comment218216> Should this class be in a separate zookeeper.admin package? src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java (line 129) <https://reviews.apache.org/r/51546/#comment218206> Please remove all red (spaces and tabs) from this file. This is mostly if not all from copying it from ZooKeeper.java. src/java/main/org/apache/zookeeper/server/DataTree.java (line 249) <https://reviews.apache.org/r/51546/#comment218217> You may additionally want to say in the comment that by default it is world readable, but not writable. I know it is documented elsewhere, but for emphasis. Actually, what happens if skipACL is set to true? Are we able to violate our contract in this case? We need to warn the user in the case reconfig and skipACL are both enabled, or possibly not even start the server. src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java (line 473) <https://reviews.apache.org/r/51546/#comment218207> This isn't really a case of bad argument, it is more like an unauthorized operation. does it make sense to create a new keeper exception for this? src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java (line 161) <https://reviews.apache.org/r/51546/#comment218219> This is doing more than just resetting zk admin, no? - fpj On Sept. 1, 2016, 4:24 p.m., Michael Han wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51546/ > --- > > (Updated Sept. 1, 2016, 4:24 p.m.) > > > Review request for zoo
Re: Review Request 25160: Major throughput improvement with mixed workloads
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25160/#review125565 --- The patch looks mostly good. I have several trivial comments, but some around synchronization that I want to understand better. I haven't looked at the tests, though, but I'll leave it for later. ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 84) <https://reviews.apache.org/r/25160/#comment188437> Why is this not final any longer? ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 95) <https://reviews.apache.org/r/25160/#comment188438> Could you align the lines of the comment pls? ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 155) <https://reviews.apache.org/r/25160/#comment188432> These comment lines are too long, please respect the 80 character limit. ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 162) <https://reviews.apache.org/r/25160/#comment188439> More long lines... ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 177) <https://reviews.apache.org/r/25160/#comment188433> Can we maintain the comment style /* */, pls? ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 188) <https://reviews.apache.org/r/25160/#comment188436> You can call just pendingRequests.put(request.sessionId, new LinkedList()); instead and remove the previous line. ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 210) <https://reviews.apache.org/r/25160/#comment188441> Why do we have to wait for empty pool both here and line 245? ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java (line 337) <https://reviews.apache.org/r/25160/#comment188440> This wakeupOnEmpty() call is synchronizing on a different object compared to wakeup(). I'm not entirely clear on how the synchronization goes here because it looks like we are now synchronizing on both this and emptyPoolSync. ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 150) <https://reviews.apache.org/r/25160/#comment188435> Long line. ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java (line 165) <https://reviews.apache.org/r/25160/#comment188434> Long line. - fpj On March 26, 2016, 7:06 p.m., Kfir Lev-Ari wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25160/ > --- > > (Updated March 26, 2016, 7:06 p.m.) > > > Review request for zookeeper, Raul Gutierrez Segales and Alexander Shraer. > > > Repository: zookeeper > > > Description > --- > > Please see https://issues.apache.org/jira/browse/ZOOKEEPER-2024 > > > Diffs > - > > ./src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java > 1726708 > > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java > 1726708 > ./src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java > 1726708 > > Diff: https://reviews.apache.org/r/25160/diff/ > > > Testing > --- > > The attached unit tests, as well as the system test found in > https://issues.apache.org/jira/browse/ZOOKEEPER-2023. > > > Thanks, > > Kfir Lev-Ari > >
Re: Review Request 31277: ZOOKEEPER-2125: SSL on Netty client-server communication
> On March 12, 2015, 9:47 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java, line > > 486 > > <https://reviews.apache.org/r/31277/diff/16/?file=890432#file890432line486> > > > > I don't understand why we have this for loop here iterating over cnxns, > > why can't we simply call closeSession on the sessionId? > > Hongchao Deng wrote: > I'm trying to keep the original logic here. > What if all ServerCnxnFactories are null (the comments said we are just > playing with diff)? Then it's possible that we won't return here and go the > the switch block. > > This is a left problem. Might be great if you can review it again. I think you can pass the request to closeSession instead of just the session id and avoid the for loop by checking the same predicate and closing the session accordingly, like in the original code. My concern with this loop is that if the number of connections is large, it might take some time. - fpj --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31277/#review76070 --- On March 13, 2015, 12:12 a.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31277/ > --- > > (Updated March 13, 2015, 12:12 a.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2125: SSL on Netty client-server communication > > > Diffs > - > > build.xml bb5ff4f70dc7cf93d42b67b80885c13fd4c4f13c > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml > 202051f1f7f517b1e1a3c561c0008449ab3c48a6 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeper.java > dd13cc9ba5096312b06999a03ae0057cd3677623 > src/java/main/org/apache/zookeeper/common/X509Exception.java PRE-CREATION > src/java/main/org/apache/zookeeper/common/X509Util.java PRE-CREATION > src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java > a97be4a5452006fbd85d355c0dcb16276cbf1c59 > src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java > acabb33f6c7a000706763ccba94cbaf5aaaca08e > src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > 41268805fe16244aeea4db3f35f13a6987b30187 > src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java > 14037722c569d560acef56de0b5a7ae13464128c > src/java/main/org/apache/zookeeper/server/ServerConfig.java > f2b8463e871739319bdf40be1f014d5ad0af5602 > src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java > 30a0ed390bb7473ddb36757da97bc7d5f4281887 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java > 0eb5c7f979199f2f7dcb9e5cfa011a9b20113713 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java > b756d349abeb1fc69534100c3633db4c1c18e031 > src/java/main/org/apache/zookeeper/server/quorum/Leader.java > 20589045752a7ba4ae9c9090055a4fcbe86a8eda > src/java/main/org/apache/zookeeper/server/quorum/Learner.java > 4dd1e947357080f3e055f3e7e2a78c979daa6ea7 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java > 388ceeb45bd18c7cb8f0766a96ebd4a54a9e76de > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > badc8df1f05dea4be337bc8312d7ac22f6c77dc3 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > d17c58d59e0131a78adde1becb5c23ce8c7a16a7 > > src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java > 2aab6d09f9bd980ed76f886fb8168aae2ac8f99f > src/java/test/data/ssl/README.md PRE-CREATION > src/java/test/data/ssl/testKeyStore.jks PRE-CREATION > src/java/test/data/ssl/testTrustStore.jks PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > 6ab19b1eb137c8b13b8ad031d474e213267da1ea > src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java > 6ce058e48d17410d89d8348ee659dd7752bfd578 > src/java/test/org/apache/zookeeper/test/ReconfigTest.java > 8b238ee7463508122010208ebc3e786caa2cf1b1 > src/java/test/org/apache/zookeeper/test/SSLTest.java PRE-CREATION > > Diff: https://reviews.apache.org/r/31277/diff/ > > > Testing > --- > > > Thanks, > > Hongchao Deng > >
Re: Review Request 31277: ZOOKEEPER-2125: SSL on Netty client-server communication
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31277/#review76070 --- It looks pretty good, I just have a few small comments and clarifications below. src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml <https://reviews.apache.org/r/31277/#comment123466> What if we change the text here to simply: the port to listen on for secure client connections using SSL. src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml <https://reviews.apache.org/r/31277/#comment123470> Don't you need to change this bit as well? src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java <https://reviews.apache.org/r/31277/#comment123807> I don't understand why we have this for loop here iterating over cnxns, why can't we simply call closeSession on the sessionId? src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java <https://reviews.apache.org/r/31277/#comment123808> why is this method synchronized? src/java/test/org/apache/zookeeper/test/SSLTest.java <https://reviews.apache.org/r/31277/#comment123811> It might be a good idea to do a CLIENT_COUNT for clarity. src/java/test/org/apache/zookeeper/test/SSLTest.java <https://reviews.apache.org/r/31277/#comment123812> Should we run an operation just to make sure the wiring on the server side is fine? - fpj On March 10, 2015, 6:04 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31277/ > --- > > (Updated March 10, 2015, 6:04 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2125: SSL on Netty client-server communication > > > Diffs > - > > build.xml bb5ff4f70dc7cf93d42b67b80885c13fd4c4f13c > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml > 202051f1f7f517b1e1a3c561c0008449ab3c48a6 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeper.java > dd13cc9ba5096312b06999a03ae0057cd3677623 > src/java/main/org/apache/zookeeper/common/X509Exception.java PRE-CREATION > src/java/main/org/apache/zookeeper/common/X509Util.java PRE-CREATION > src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java > a97be4a5452006fbd85d355c0dcb16276cbf1c59 > src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java > acabb33f6c7a000706763ccba94cbaf5aaaca08e > src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > 41268805fe16244aeea4db3f35f13a6987b30187 > src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java > 14037722c569d560acef56de0b5a7ae13464128c > src/java/main/org/apache/zookeeper/server/ServerConfig.java > f2b8463e871739319bdf40be1f014d5ad0af5602 > src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java > 30a0ed390bb7473ddb36757da97bc7d5f4281887 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java > 0eb5c7f979199f2f7dcb9e5cfa011a9b20113713 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java > b756d349abeb1fc69534100c3633db4c1c18e031 > src/java/main/org/apache/zookeeper/server/quorum/Leader.java > 20589045752a7ba4ae9c9090055a4fcbe86a8eda > src/java/main/org/apache/zookeeper/server/quorum/Learner.java > 4dd1e947357080f3e055f3e7e2a78c979daa6ea7 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java > 388ceeb45bd18c7cb8f0766a96ebd4a54a9e76de > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > badc8df1f05dea4be337bc8312d7ac22f6c77dc3 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > d17c58d59e0131a78adde1becb5c23ce8c7a16a7 > > src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java > 2aab6d09f9bd980ed76f886fb8168aae2ac8f99f > src/java/test/data/ssl/README.md PRE-CREATION > src/java/test/data/ssl/testKeyStore.jks PRE-CREATION > src/java/test/data/ssl/testTrustStore.jks PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > 6ab19b1eb137c8b13b8ad031d474e213267da1ea > src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java > 6ce058e48d17410d89d8348ee659dd7752bfd578 > src/java/test/org/apache/zookeeper/test/ReconfigTest.java > 8b238ee7463508122010208ebc3e786caa2cf1b1 > src/java/test/org/apache/zookeeper/test/SSLTest.java PRE-CREATION > > Diff: https://reviews.apache.org/r/31277/diff/ > > > Testing > --- > > > Thanks, > > Hongchao Deng > >
Re: Review Request 30576: ZOOKEEPER-2094: SSL on Netty
> On Feb. 4, 2015, 4:02 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 1107 > > <https://reviews.apache.org/r/30576/diff/1/?file=846510#file846510line1107> > > > > Is there any use case in which we need this? Could you explain? > > Ian Dimayuga wrote: > This is to enable the "super" scheme the same way the > DigestAuthenticationProvider does. It's so that an admin can inspect/modify > the tree even when a client has set very restrictive ACLs. I guess I'm confused about the context here. This feature is about secure connections via SSL, so it is about connecting, not about inspecting the data in the ZK state, but I guess I'm missing something here. > On Feb. 4, 2015, 4:02 p.m., fpj wrote: > > src/java/test/org/apache/zookeeper/test/SSLAuthTest.java, line 20 > > <https://reviews.apache.org/r/30576/diff/1/?file=846539#file846539line20> > > > > This isn't working for me, it seems to be looking for > > src/java/test/data/auth instead of build/test/data. > > Ian Dimayuga wrote: > test.data.dir is set by the ant target "junit.run" in a > tag. It should be working exactly the same way the InvalidSnapshotTest works. > Have you downloaded the .jks files from the JIRA? Sure, this is ok, unless we decide to have the files somewhere else as I suggested in the jira. - fpj --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30576/#review70954 --- On Feb. 3, 2015, 9 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30576/ > --- > > (Updated Feb. 3, 2015, 9 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > SSL on Netty > > > Diffs > - > > build.xml e4c8e0374b25a5bcab7cfe77543378fdb8f98b06 > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml > 914a24471b0a27f7cf650c3ed2eef1077178853f > src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml > 223cf8e5a856aefba6f5c106d3f4861d3de8f1e1 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java > 87e7834bc91c52d4a2d100dbcc98d41a1b98b849 > src/java/main/org/apache/zookeeper/ZooKeeper.java > dd13cc9ba5096312b06999a03ae0057cd3677623 > src/java/main/org/apache/zookeeper/ZooKeeperMain.java > 496e88748cf6aa291c8b71583a28fdb55fdf7761 > src/java/main/org/apache/zookeeper/auth/X509Auth.java PRE-CREATION > src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java > e41465ab93a3a59dbced8294e83b1651ad0dfe69 > src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java > e02753f4fb926a8cc6c7a7c10af42f949c1e210c > src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java > acabb33f6c7a000706763ccba94cbaf5aaaca08e > src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java > b4bdc82f8b52f736c6ee3d67bb793a3616c1b436 > src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > 09a794844978456fc3580adc22b6064e2a12cf77 > src/java/main/org/apache/zookeeper/server/ServerCnxn.java > a47d85662970cc0c219a46b226737a8689f8fe96 > src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java > 14037722c569d560acef56de0b5a7ae13464128c > src/java/main/org/apache/zookeeper/server/ServerConfig.java > f2b8463e871739319bdf40be1f014d5ad0af5602 > src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java > 30a0ed390bb7473ddb36757da97bc7d5f4281887 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java > 0eb5c7f979199f2f7dcb9e5cfa011a9b20113713 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java > b756d349abeb1fc69534100c3633db4c1c18e031 > src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java > 406015f84a51e6afcfe704b881f8494bdd687a25 > > src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/Leader.java > 20589045752a7ba4ae9c9090055a4fcbe86a8eda > src/java/main/org/apache/zookeeper/server/quorum/Learner.java > 87f4c0627141c2cfee0533aca7ba2e7ff91433e3 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java > 388ceeb45bd18c7cb8f0766a96ebd4a54a9e76de > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > badc8df1f05dea4be337bc8312d7ac22f6c77dc3 > src/
Re: Review Request 30576: ZOOKEEPER-2094: SSL on Netty
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30576/#review70954 --- This is a first pass on the patch. It looks good and I just have a few issues below. I have more on the jira, though. src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml <https://reviews.apache.org/r/30576/#comment116422> I'd rather have separate entries in the docs for clientPort and secureClientPort, and a note to explain mixed mode when both are set. src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml <https://reviews.apache.org/r/30576/#comment116423> Is there any use case in which we need this? Could you explain? src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/30576/#comment116424> Spurious whitespaces. Please check the red marks throughout. src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java <https://reviews.apache.org/r/30576/#comment116427> Given that the call without the boolen parameter is still there, why not just call it sed4LetterWordSecure and remove the boolean parameter? src/java/main/org/apache/zookeeper/server/quorum/Learner.java <https://reviews.apache.org/r/30576/#comment116428> Check for whitespaces. src/java/test/org/apache/zookeeper/test/SSLAuthTest.java <https://reviews.apache.org/r/30576/#comment116434> This isn't working for me, it seems to be looking for src/java/test/data/auth instead of build/test/data. src/java/test/org/apache/zookeeper/test/SSLAuthTest.java <https://reviews.apache.org/r/30576/#comment116432> let's fail the test in the case the create call doesn't go through. - fpj On Feb. 3, 2015, 9 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30576/ > --- > > (Updated Feb. 3, 2015, 9 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > SSL on Netty > > > Diffs > - > > build.xml e4c8e0374b25a5bcab7cfe77543378fdb8f98b06 > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml > 914a24471b0a27f7cf650c3ed2eef1077178853f > src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml > 223cf8e5a856aefba6f5c106d3f4861d3de8f1e1 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java > 87e7834bc91c52d4a2d100dbcc98d41a1b98b849 > src/java/main/org/apache/zookeeper/ZooKeeper.java > dd13cc9ba5096312b06999a03ae0057cd3677623 > src/java/main/org/apache/zookeeper/ZooKeeperMain.java > 496e88748cf6aa291c8b71583a28fdb55fdf7761 > src/java/main/org/apache/zookeeper/auth/X509Auth.java PRE-CREATION > src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java > e41465ab93a3a59dbced8294e83b1651ad0dfe69 > src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java > e02753f4fb926a8cc6c7a7c10af42f949c1e210c > src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java > acabb33f6c7a000706763ccba94cbaf5aaaca08e > src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java > b4bdc82f8b52f736c6ee3d67bb793a3616c1b436 > src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > 09a794844978456fc3580adc22b6064e2a12cf77 > src/java/main/org/apache/zookeeper/server/ServerCnxn.java > a47d85662970cc0c219a46b226737a8689f8fe96 > src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java > 14037722c569d560acef56de0b5a7ae13464128c > src/java/main/org/apache/zookeeper/server/ServerConfig.java > f2b8463e871739319bdf40be1f014d5ad0af5602 > src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java > 30a0ed390bb7473ddb36757da97bc7d5f4281887 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java > 0eb5c7f979199f2f7dcb9e5cfa011a9b20113713 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java > b756d349abeb1fc69534100c3633db4c1c18e031 > src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java > 406015f84a51e6afcfe704b881f8494bdd687a25 > > src/java/main/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/Leader.java > 20589045752a7ba4ae9c9090055a4fcbe86a8eda > src/java/main/org/apache/zookeeper/server/quorum/Learner.java > 87f4c0627141c2cfee0533aca7ba2e7ff91433e3 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java > 388ceeb45bd18c7cb8f0766a96ebd4a54a9e76de
Re: Review Request 27244: ZOOKEEPER-2069
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134>
> >
> > It sounds like making pendingQueue and outgoingQueue are optimizations
> > that are independent from this issue. Shouldn't we make this change in a
> > different jira?
>
> Hongchao Deng wrote:
> outgoingQueue isn't optimization... My previous patch used a semaphore
> which will be waken up by WakeupCnxn(). Your suggestion is use a blocking
> outgoingQueue. What do you think? I am fine since I know both changes well
> and I can do it in separate JIRAs.
>
> fpj wrote:
> My concern here is that it ended up implying more changes than just
> removing the semaphore. For example, the patch removes this synchronized
> block (not sure it is going to format it right):
>
> for (Packet p : outgoingQueue) {
>conLossPacket(p);
> }
> outgoingQueue.clear();
>
> and checking the other synchronized blocks removed, it isn't entirely
> straightforward that you can remove all of them.
>
> Hongchao Deng wrote:
> I can add those synchronized back. However, findbugs will complain that
> so I will add rules too.
>
> On Netty side I can assure your concern:
> 1. In ClientCnxn, primeConnection(), cleanup(), queuePacket().
> primeConnection(), cleanup() is guaranteed to be start and end of life
> cycles. queuePacket() is appending packets to outgoingQueue and waking up the
> blocking queue.
>
>
> What do you think?
I'm not suggesting that you bring back the synchronized blocks. I'm saying that
I haven't checked carefully and it isn't straightforward that you can remove
them. For example, the one in ClientCnxnSocketNIO line 163 has multiple
accesses to outgoingQueue, so removing the synchronized block doesn't
necessarily gives us the same behavior. Can you actually justify the removal of
each one of those synchronized blocks? If you aren't sure, then I'll check it
carefully.
- fpj
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
---
On Dec. 12, 2014, 7:16 p.m., Hongchao Deng wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> ---
>
> (Updated Dec. 12, 2014, 7:16 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> ---
>
> ZOOKEEPER-2069
>
>
> Diffs
> -
>
> build.xml bb5ff4f
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/main/org/apache/zookeeper/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java
> PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java
> PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java
> PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> ---
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1472
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line1472>
> >
> > I'm now confused by this change. Are you getting rid of enableWrite?
> > saslCompleted is fairly different from enableWrite.
>
> Hongchao Deng wrote:
> So the guy who make the SASL change do it in a hackish way. It simply
> blocks the entire NIO thread and send it directly through socket. But when it
> finishes SASL, it calls enableWrite(). I can definitely use the old name
> enableWrite(). But do you think it is better to drive the change here? Since
> Netty client doesn't need to enable write here.
>
> Hongchao Deng wrote:
> Hi Flavio. Maybe you mean enableWrite() is also used by CilentCnxnNIO and
> worried that I change the name as well. I only change the interface to
> outside. Inside ClientCnxnNIO there is still a enableWrite function which is
> used inside the class's other places.
Yes, you're right, I did confuse the calls.
> On Dec. 11, 2014, 11:16 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134
> > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134>
> >
> > It sounds like making pendingQueue and outgoingQueue are optimizations
> > that are independent from this issue. Shouldn't we make this change in a
> > different jira?
>
> Hongchao Deng wrote:
> outgoingQueue isn't optimization... My previous patch used a semaphore
> which will be waken up by WakeupCnxn(). Your suggestion is use a blocking
> outgoingQueue. What do you think? I am fine since I know both changes well
> and I can do it in separate JIRAs.
My concern here is that it ended up implying more changes than just removing
the semaphore. For example, the patch removes this synchronized block (not sure
it is going to format it right):
for (Packet p : outgoingQueue) {
conLossPacket(p);
}
outgoingQueue.clear();
and checking the other synchronized blocks removed, it isn't entirely
straightforward that you can remove all of them.
- fpj
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27244/#review64807
---
On Dec. 12, 2014, 7:16 p.m., Hongchao Deng wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27244/
> ---
>
> (Updated Dec. 12, 2014, 7:16 p.m.)
>
>
> Review request for zookeeper.
>
>
> Repository: zookeeper-git
>
>
> Description
> ---
>
> ZOOKEEPER-2069
>
>
> Diffs
> -
>
> build.xml bb5ff4f
> src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07
> src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee
> src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION
> src/java/main/org/apache/zookeeper/ZooKeeperTestable.java 775d1a2
> src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080
> src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java
> PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java
> PRE-CREATION
> src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java
> PRE-CREATION
>
> Diff: https://reviews.apache.org/r/27244/diff/
>
>
> Testing
> ---
>
> 1. use LinkedBlockingDeque.
>
>
> Thanks,
>
> Hongchao Deng
>
>
Re: Review Request 27244: ZOOKEEPER-2069
> On Dec. 11, 2014, 10:53 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 70 > > <https://reviews.apache.org/r/27244/diff/28/?file=789550#file789550line70> > > > > I see, the previous sasl changes are necessary here so that we can > > proceed only if authentication suceeds. Please confirm. > > Hongchao Deng wrote: > I change enableWrite(), enableReadWriteOnly(), WakeupCnxn() interfaces: > 1. enable read, write is renaming only. Because in netty it doesn't make > sense to say enable write. It's more natural to say SASL completed and what > we are doing here. > 2. I split WakeupCnxn() into two because they are for two purposes: one > is for notifying packet has been added to outgoingQueue, the other is for > notifying connection is intentionally closed. They are different in Netty. Ok, got it. Netty 3 actually allowed you to control those flags directly, but apparently they got rid of it in Netty 4. See this http://stackoverflow.com/questions/24888912/where-is-channel-setinterestops-in-netty-4x I think you're saying that the enableWrite call was there only for sasl so you prefer to just name explicitly? If that's the intention, then seems good to me. > On Dec. 11, 2014, 10:53 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 68 > > <https://reviews.apache.org/r/27244/diff/28/?file=789549#file789549line68> > > > > Why does pendingQueue need to be made concurrent? > > Hongchao Deng wrote: > I will convert it back. If it is worth having this optmization, then I suggest you propose it in another jira. You have the changes already, it should be easy to generate a patch. > On Dec. 11, 2014, 10:53 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1372 > > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line1372> > > > > If these SASL changes aren't strictly necessary for this patch, we > > should do them in a different jira. > > Hongchao Deng wrote: > This is commented by Raul @rgs to change.. I usually didn't change name > myself. My question wasn't about the name change, but instead about the changes around sasl. I got what you're trying to do now. - fpj --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review64803 --- On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > --- > > (Updated Dec. 11, 2014, 6:11 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2069 > > > Diffs > - > > build.xml bb5ff4f > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeperTestable.java 775d1a2 > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080 > src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > --- > > 1. use LinkedBlockingDeque. > > > Thanks, > > Hongchao Deng > >
Re: Review Request 27244: ZOOKEEPER-2069
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review64807 --- A few more, I'm not sorry for sending them separately. src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/27244/#comment107583> It sounds like making pendingQueue and outgoingQueue are optimizations that are independent from this issue. Shouldn't we make this change in a different jira? src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/27244/#comment107584> I'm now confused by this change. Are you getting rid of enableWrite? saslCompleted is fairly different from enableWrite. src/java/test/org/apache/zookeeper/test/ClientTest.java <https://reviews.apache.org/r/27244/#comment107585> Good catch. - fpj On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > --- > > (Updated Dec. 11, 2014, 6:11 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2069 > > > Diffs > - > > build.xml bb5ff4f > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeperTestable.java 775d1a2 > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080 > src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > --- > > 1. use LinkedBlockingDeque. > > > Thanks, > > Hongchao Deng > >
Re: Review Request 27244: ZOOKEEPER-2069
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review64803 --- A few more comments, Hongchao. Thanks for the changes so far. build.xml <https://reviews.apache.org/r/27244/#comment107568> Is this accidental? There is an issue open to change it to 1.7. If it is necessary, we can make this change here, but otherwise leave it for the other patch. src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/27244/#comment107569> If you end up generating a new patch, please remove this tab/spaces. src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/27244/#comment107570> If these SASL changes aren't strictly necessary for this patch, we should do them in a different jira. src/java/main/org/apache/zookeeper/ClientCnxn.java <https://reviews.apache.org/r/27244/#comment107571> See previous comment. src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java <https://reviews.apache.org/r/27244/#comment107572> Why does pendingQueue need to be made concurrent? src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java <https://reviews.apache.org/r/27244/#comment107573> TODOs require a jira number. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment107577> I see, the previous sasl changes are necessary here so that we can proceed only if authentication suceeds. Please confirm. - fpj On Dec. 11, 2014, 6:11 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > --- > > (Updated Dec. 11, 2014, 6:11 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2069 > > > Diffs > - > > build.xml bb5ff4f > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeperTestable.java 775d1a2 > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080 > src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > --- > > 1. use LinkedBlockingDeque. > > > Thanks, > > Hongchao Deng > >
Re: Review Request 27244: ZOOKEEPER-2069
> On Dec. 2, 2014, 11:43 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188 > > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188> > > > > Could you elaborate on what you're trying to do with this NIOLock? I'm > > not sure what you mean with race cocnditions in the comment. > > Hongchao Deng wrote: > 1. The intuition is that in NIO client all reads and writes are > sequential. > 2. One problem I found: (1) packet is removed from outgoingQueue; (2) > before the packet is added to pendingQueue; (3) sendThread.readResponse > throws exception. > 3. I am wondering what the purpose of pendingQueue is except for checking > and if we can remove it. AFAICT that's the only concurrency concern. > > Hongchao Deng wrote: > I come up with a better way if the pendingQueue is the only concern. It > looks like pendingQueue doesn't care about ordering. So I can add packets to > pendingQueue before writing to channel. > > fpj wrote: > Check SendThread.readResponse() in ClientCnxn, pls. > > Hongchao Deng wrote: > I did. I figure out it holds the assumptions: > 1. pendingQueue only takes non-priming packets. > 2. pendingQueue ordering is the same as packets sent in. > > I would re-clarify my point that I can remove the lock as long as the > only interaction between read and write is the pendingQueue. :) I've been revisiting the code and I can't think of any other potential problem. - fpj --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review63604 --- On Dec. 3, 2014, 10:20 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > --- > > (Updated Dec. 3, 2014, 10:20 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2069 > > > Diffs > - > > build.xml bb5ff4f > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeper.java dd13cc9 > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > --- > > 1. use LinkedBlockingDeque. > > > Thanks, > > Hongchao Deng > >
Re: Review Request 27244: ZOOKEEPER-2069
> On Dec. 2, 2014, 11:43 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 66 > > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line66> > > > > Is the reason to have workSemaphore to block in doTransport when there > > isn't anything in the outgoing queue? If so, we might be better off with a > > blocking queue rather than a list as we have right now. > > Hongchao Deng wrote: > Sounds good on my side. I tried to make minimal changes. Do you think I > should make it in here or create another JIRA? I'd rather have it fixed here, but depends on how much more energy you have to put into this patch. > On Dec. 2, 2014, 11:43 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 107 > > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line107> > > > > I'm not sure why you're not using the ChannelFuture returned here. See > > this connect() method in bookkeeper for an example > > https://github.com/apache/bookkeeper/blob/branch-4.0/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L112 > > Hongchao Deng wrote: > Good. Fix it next Ok. > On Dec. 2, 2014, 11:43 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 188 > > <https://reviews.apache.org/r/27244/diff/22/?file=774526#file774526line188> > > > > Could you elaborate on what you're trying to do with this NIOLock? I'm > > not sure what you mean with race cocnditions in the comment. > > Hongchao Deng wrote: > 1. The intuition is that in NIO client all reads and writes are > sequential. > 2. One problem I found: (1) packet is removed from outgoingQueue; (2) > before the packet is added to pendingQueue; (3) sendThread.readResponse > throws exception. > 3. I am wondering what the purpose of pendingQueue is except for checking > and if we can remove it. AFAICT that's the only concurrency concern. > > Hongchao Deng wrote: > I come up with a better way if the pendingQueue is the only concern. It > looks like pendingQueue doesn't care about ordering. So I can add packets to > pendingQueue before writing to channel. Check SendThread.readResponse() in ClientCnxn, pls. - fpj --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review63604 --- On Dec. 3, 2014, 1:52 a.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > --- > > (Updated Dec. 3, 2014, 1:52 a.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2069 > > > Diffs > - > > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > --- > > 1. use ChannelFuture returned by connect() > 2. remove NIOLock > > > Thanks, > > Hongchao Deng > >
Re: Review Request 27244: ZOOKEEPER-2069
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review63604 --- Looks better, Hongchao, thanks. I have a few more comments below, hope you don't mind. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment105881> Is the reason to have workSemaphore to block in doTransport when there isn't anything in the outgoing queue? If so, we might be better off with a blocking queue rather than a list as we have right now. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment105880> I'm not sure why you're not using the ChannelFuture returned here. See this connect() method in bookkeeper for an example https://github.com/apache/bookkeeper/blob/branch-4.0/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L112 src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment105876> Could you elaborate on what you're trying to do with this NIOLock? I'm not sure what you mean with race cocnditions in the comment. - fpj On Nov. 24, 2014, 7:01 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > --- > > (Updated Nov. 24, 2014, 7:01 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2069 > > > Diffs > - > > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > --- > > > Thanks, > > Hongchao Deng > >
Re: Review Request 27244: ZOOKEEPER-2069
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review59363 --- First batch of comments. The overall structure looks good, but I have a few questions and comments. src/java/main/org/apache/zookeeper/ClientCnxnSocket.java <https://reviews.apache.org/r/27244/#comment100641> "... already started sending...". Also, the comment is overflowing, so moving it to the top sounds like a good choice. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment100745> Shouldn't isConnected return true in the case channel isn't null instead of channelFactory? In fact, it sounds like this implementation is instantiating a new ChannelFactory every time it tries to connect (modulo an attempt already being in progress). It doesn't sound necessary. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment100747> Could you use the future object it returns as opposed to using the firstConnect CountDownLatch? I think you're using the latch to determine if the connection request is still pending. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/27244/#comment100746> A message like "outgoingQueue isn't empty, but we haven't been notified". Also, I'm not sure how this could happen, is this a potential race or just a sanity check? - fpj On Oct. 31, 2014, 10:57 p.m., Hongchao Deng wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > --- > > (Updated Oct. 31, 2014, 10:57 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > --- > > ZOOKEEPER-2069 > > > Diffs > - > > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeper.java dd13cc9 > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > --- > > > Thanks, > > Hongchao Deng > >
RE: ZooKeeper 3.5.0-alpha planning
+1 for having an RC this week. Since this is an alpha release, I think 72 biz hours is enough for the vote. -Flavio > -Original Message- > From: Patrick Hunt [mailto:ph...@apache.org] > Sent: 21 July 2014 18:55 > To: DevZooKeeper > Subject: Re: ZooKeeper 3.5.0-alpha planning > > I fixed a number of issues. I also started a few threads with builds@ > - the ulimit issue is still outstanding. Hongchao and I worked through a > number of findbugs issues, it's not closed yet but it's pretty close. > > I don't see why we can't create an RC and start voting this week though. > Anyone disagree? > > How long should we let the vote run, the std 72 biz hours or should we plan > for more to allow folks more time to test? > > Patrick > > On Mon, Jul 21, 2014 at 10:29 AM, Raúl Gutiérrez Segalés > wrote: > > On 18 July 2014 10:32, Patrick Hunt wrote: > > > >> You may notice some back/forth on Apache Jenkins ZK jobs - I'm trying > >> to fix some of the jobs that were broken during the recent host > >> upgrade. > >> > > > > How are things looking? Is it likely that we can have a 3.5.0 alpha > > release week or are we still blocked on Jenkins? > > > > > > -rgs > > > > > > > > > > > > > >> Patrick > >> > >> On Thu, Jul 17, 2014 at 1:47 PM, Michi Mutsuzaki > >> > >> wrote: > >> > I'll check in ZOOKEEPER-1683. > >> > > >> > On Thu, Jul 17, 2014 at 11:20 AM, Alexander Shraer > >> > > >> wrote: > >> >> can we also have ZOOKEEPER-1683 in ? Camille gave a +1 and all > >> subsequent > >> >> changes were formatting as suggested by Rakesh. > >> >> > >> >> > >> >> On Thu, Jul 17, 2014 at 9:48 AM, Patrick Hunt > wrote: > >> >> > >> >>> I'm concerned that the CI tests are all failing due to, for e.g. > >> >>> findbugs issues. At the very least our build/test/ci should be > >> >>> pretty clean - some flakeys is ok (the recent startServer fix and > >> >>> some other flakeys that have been addressed go a long way on that > >> >>> issue) but I think the findbugs problem should be cleaned up > >> >>> before we cut a release. I started a separate thread to discuss the > findbugs issue. > >> >>> > >> >>> Otw we seem to be in ok shape - 1863 is in. > >> >>> > >> >>> Anyone have a chance to give feedback to Raul on 1919? > >> >>> > >> >>> Patrick > >> >>> > >> >>> On Tue, Jul 15, 2014 at 10:34 AM, Flavio Junqueira > >> >>> wrote: > >> >>> > My take: > >> >>> > > >> >>> > - ZK-1863 is pending review. It is a blocker and it can go in. > >> >>> > See > >> the > >> >>> jira for comments. > >> >>> > - We can try to have ZK-1807 in for the first alpha. > >> >>> > - I'd rather not have the first alpha depending on ZK-1919 and > >> ZK-1910, > >> >>> we can leave it for the second alpha. > >> >>> > > >> >>> > If you agree with this, then we should be able to cut a > >> >>> > candidate by > >> the > >> >>> end of this week. > >> >>> > > >> >>> > -Flavio > >> >>> > > >> >>> > On 15 Jul 2014, at 17:26, Patrick Hunt wrote: > >> >>> > > >> >>> >> Per my previous note you can now see the c client test log > >> >>> >> output > >> here > >> >>> >> in the "build artifacts" section: > >> >>> >> > >> >>> > >> https://builds.apache.org/view/S-Z/view/ZooKeeper/job/ZooKeeper- > trunk > >> /2372/ > >> >>> >> > >> >>> >> Patrick > >> >>> >> > >> >>> >> On Mon, Jul 14, 2014 at 7:36 PM, Patrick Hunt > >> >>> >> > >> wrote: > >> >>> >>> Update: we're back to 8 blockers on 3.5.0 (not clear to me > >> >>> >>> which > >> >>> >>> one(s?) is new?) > >> >>> >>> > >> >>> >>> Looks like the autoconf issue I reported is hitting the > >> >>> >>> upgraded apache jenkins instances as well. I've updated the > >> >>> >>> "archive" list > >> to > >> >>> >>> include the c tests stdout redirect. So while it won't go to > >> console > >> >>> >>> at least we can debug when there is a failure. > >> >>> >>> > >> >>> >>> Raul has been helping Bill with reviews for the jetty server > >> support > >> >>> >>> and it looks like that should be ready soon. > >> >>> >>> > >> >>> >>> Raul also requested that someone prioritize reviewing > >> "ZOOKEEPER-1919 > >> >>> >>> Update the C implementation of removeWatches to have it > match > >> >>> >>> ZOOKEEPER-1910" so that we can include it in 3.5.0. Flavio/Michi? > >> >>> >>> > >> >>> >>> Hongchao got a patch in to cleanup the flakey c client > >> >>> >>> reconfig > >> test - > >> >>> >>> kudos on helping cleanup the build/test infra! > >> >>> >>> > >> >>> >>> > >> >>> >>> Based on previous comments it looks like we're pretty close. > >> >>> >>> Do > >> folks > >> >>> >>> feel comfortable with a 3.5.0 alpha at this point? (with a > >> >>> >>> few > >> pending > >> >>> >>> as above) > >> >>> >>> > >> >>> >>> Patrick > >> >>> >>> > >> >>> >>> On Fri, Jul 11, 2014 at 9:24 AM, Raúl Gutiérrez Segalés > >> >>> >>> wrote: > >> >>> On Jul 11, 2014 6:37 AM, "Flavio Junqueira" > >> >>> > >> >>> wrote: > >> >>> > > >> >>> > Just so that we don´t delay too much, what if we release an > >> >>> > alpha > >> >>> version >
svn upgrade issue
The precommit build is getting this error: [exec] svn: E155036: Please see the 'svn upgrade' command [exec] svn: E155036: The working copy at '/home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-Build/trunk' [exec] is too old (format 10) to work with client version '1.8.8 (r1568071)' (expects format 31). You need to upgrade the working copy first. We need to get it upgraded. What's the way to do it? -Flavio
RE: c client test output seems to be broken for trunk
Yeah, I was pretty much sure we had a jira for this latest failure, but I couldn't find it... -Flavio > -Original Message- > From: Patrick Hunt [mailto:ph...@apache.org] > Sent: 08 July 2014 17:00 > To: DevZooKeeper > Subject: Re: c client test output seems to be broken for trunk > > The latter - test-core-cppunit. > > Patrick > > On Tue, Jul 8, 2014 at 8:56 AM, Raúl Gutiérrez Segalés > wrote: > > On 7 July 2014 23:18, Patrick Hunt wrote: > > > >> The test output for the c client seems to be broken. afaict this is a > >> recent change because the jenkins console output shows the test > >> details, but I don't see them in the latest trunk. > >> > >> I now see this (along with a long pause) > >> [exec] make[1]: Entering directory > >> `/home/phunt/dev/zookeeper-trunk/build/test/test-cppunit' > >> [exec] make[2]: Entering directory > >> `/home/phunt/dev/zookeeper-trunk/build/test/test-cppunit' > >> [exec] PASS: zktest-st > >> > >> whereas before we used to see output such as: > >> ZooKeeper server startedRunning > >> Zookeeper_init::testBasic : elapsed 1 : OK > >> Zookeeper_init::testAddressResolution : elapsed 0 : OK > >> Zookeeper_init::testMultipleAddressResolution : elapsed 0 : OK > >> Zookeeper_init::testNullAddressString : elapsed 0 : OK > >> Zookeeper_init::testEmptyAddressString : elapsed 0 : OK > >> > >> and so on. Which is now missing. > >> > >> Anyone know what happened here? It would be nice if we could turn > >> this back on - so that you get progress information as the test runs > >> (also captured in the jenkins console). > >> > > > > Is this running make check-TESTS manually or ant test-core-cppunit? > > > > -rgs
Re: Review Request 23081: ZOOKEEPER-827. enable r/o mode in C client library
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23081/#review46948 --- Ship it! Just a few minor comments, otherwise it looks good. src/c/include/zookeeper.h <https://reviews.apache.org/r/23081/#comment82517> As I understand it, this code indicates that a write operation was tried when connected to a read-only server. If this is right, then the code should be something like ZISREADONLY or simply ZREADONLY. src/c/src/addrvec.c <https://reviews.apache.org/r/23081/#comment82518> Just a style comment, I'd rather use curly braces even if there is just a single line of code for the if. src/c/src/zookeeper.c <https://reviews.apache.org/r/23081/#comment82519> Same here, I'd rather use curly braces even if not necessary. - fpj On June 27, 2014, 6:45 p.m., Raul Gutierrez Segales wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23081/ > --- > > (Updated June 27, 2014, 6:45 p.m.) > > > Review request for zookeeper, fpj, michim, Patrick Hunt, Rakesh R, and Thawan > Kooburat. > > > Bugs: ZOOKEEPER-827 > https://issues.apache.org/jira/browse/ZOOKEEPER-827 > > > Repository: zookeeper-git > > > Description > --- > > commit e93806d0a6dfd8b372b0557fb5dfbbaca6ae8178 > Author: Raul Gutierrez S > Date: Sun Jun 8 21:43:48 2014 -0700 > > ZOOKEEPER-827. enable r/o mode in C client library > > Signed-off-by: Raul Gutierrez S > > :100644 100644 6130c4d... a7b2621... Msrc/c/Makefile.am > :100644 100644 fe25015... 045ef9a... Msrc/c/README > :100644 100644 d5a3876... 18a203d... Msrc/c/include/zookeeper.h > :100644 100644 4134044... c6887f5... Msrc/c/src/addrvec.c > :100644 100644 5345ebc... 3897095... Msrc/c/src/addrvec.h > :100644 100644 aab214d... a9d11e1... Msrc/c/src/cli.c > :100644 100644 0d3ae19... 0c513e8... Msrc/c/src/load_gen.c > :100644 100644 21dc70c... eec505c... Msrc/c/src/zk_adaptor.h > :100644 100644 795875e... 3cc9393... Msrc/c/src/zookeeper.c > :100644 100644 41d5179... c6536dc... Msrc/c/tests/TestClientRetry.cc > :00 100644 000... 37ab147... A > src/c/tests/TestReadOnlyClient.cc > :00 100644 000... 035f26d... Asrc/c/tests/WatchUtil.h > :100644 100644 f97c943... 263f3ce... Msrc/c/tests/ZKMocks.cc > :100644 100644 fbcfc4f... 2717ded... Msrc/c/tests/ZKMocks.h > :00 100644 000... aa2dd8c... Asrc/c/tests/quorum.cfg > :100755 100755 0af80f8... 33ab152... Msrc/c/tests/zkServer.sh > > > Diffs > - > > src/c/Makefile.am 6130c4d > src/c/README fe25015 > src/c/include/zookeeper.h d5a3876 > src/c/src/addrvec.h 5345ebc > src/c/src/addrvec.c 4134044 > src/c/src/cli.c aab214d > src/c/src/load_gen.c 0d3ae19 > src/c/src/zk_adaptor.h 21dc70c > src/c/src/zookeeper.c 795875e > src/c/tests/TestClientRetry.cc 41d5179 > src/c/tests/TestReadOnlyClient.cc PRE-CREATION > src/c/tests/WatchUtil.h PRE-CREATION > src/c/tests/ZKMocks.h fbcfc4f > src/c/tests/ZKMocks.cc f97c943 > src/c/tests/quorum.cfg PRE-CREATION > src/c/tests/zkServer.sh 0af80f8 > > Diff: https://reviews.apache.org/r/23081/diff/ > > > Testing > --- > > all tests pass, *should* compile on Windows. > > > Thanks, > > Raul Gutierrez Segales > >
