Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/566
lgtm. +1, thanks @cpoerschke .
Perhaps consider logging the value during startup (initial read of the
value) instead?
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/47
@cpoerschke please close this given your recent comment. thanks.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/350
@cpoerschke can you close this PR given your comment re #554 and the fact
that it's been committed? Thanks.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/575
+1, LGTM. Nice catch @sl4mmy . Consider submitting a PR to add a comment to
the bottom of ZOO_ERRORS warning folks that they should also update zerror(...)
if they change/add to the enum.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/557
+1, LGTM. Worked fine testing on ubuntu/master. Thanks @sl4mmy !
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/576
Seems worth a try, +1. Thanks @lavacat . Let's continue to keep an eye on
this one.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/466
+1, lgtm. Also tried running against a runtime cluster and it worked ok.
committed to 3.5.5 and master. Thanks @anmolnar .
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/466
Currently listed as having a conflict, @anmolnar can you update the patch?
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/528
Hm. iiuc the m4 checks are during configure script creation itself, so
perhaps that's not the best idea either. :-(
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/528
Perhaps we could do this? -- check if the cppunit macro is defined, if not
then check for pkg-config, if not then error out (or whatever we do today). otw
use the first macro/approach available.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/528
Hm. It's not very clear that
> ./configure: line 5034: PKG_PROG_PKG_CONFIG: command not found
means that pkg-config needs to be installed. Esp given we've never needed
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/467
+1 lgtm. I was able to run the coverage successfully and view the report.
possible followups:
1) The report/results doesn't look great - only 50% coverage.
2) I h
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/528
I'm afraid it still doesn't work for me. This is ubuntu 14.04 - the docker
image I pointed to previously.
# ./configure
checking for doxygen... no
checking for pe
Github user phunt closed the pull request at:
https://github.com/apache/zookeeper/pull/537
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/537
This is specifically for 3.4 - see #536 for 3.5/master.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/536
I'm afraid conflict on 3.4 (fine for 3.5 though) - see #537 for branch-3.4
PR
---
GitHub user phunt opened a pull request:
https://github.com/apache/zookeeper/pull/537
ZOOKEEPER-2920: Upgrade OWASP Dependency Check to 3.2.1
Updated and verified on all the active branches.
Change-Id: I750d7e576e8c731aab4cef26f6863eac6b1b8dc2
(cherry picked from commit
GitHub user phunt opened a pull request:
https://github.com/apache/zookeeper/pull/536
ZOOKEEPER-2920: Upgrade OWASP Dependency Check to 3.2.1
Updated and verified on all the active branches.
Change-Id: I750d7e576e8c731aab4cef26f6863eac6b1b8dc2
You can merge this pull
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/531
+1, lgtm. Thanks @timothyjward !
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/532
+1, lgtm. Thanks @eolivelli
Please close this PR manually as it's not the default branch for the repo.
thx.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/524
+1, lgtm. Thanks @eolivelli
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/528
ps. you can try using my repos, makes it easy to try out various envs:
https://hub.docker.com/r/phunt/
See the readme on how to run - I typically map my zk source directory into
the
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/528
Seems like it breaks ubuntu 14.04 (not sure what else, that's what I
tried). This is working fine w/o the patch:
./configure
checking for doxygen... no
checking for
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/511
@roy2220 now that #522 is committed can you give it a try and verify?
Thanks!
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/522
Hm. I had some trouble testing this. In particular I ran into the
following. Some of this might be due to the fact that I run ubuntu in a docker
container on my mac, but I think it could be an
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/524
@eolivelli something seems borked - there are 8 changed files, many of
which seem unrelated to the issue. Perhaps you pulled in some unexpected
changes accidentally?
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/527
Done. Thanks @hanm
---
GitHub user phunt opened a pull request:
https://github.com/apache/zookeeper/pull/527
ZOOKEEPER-3051: Updated jackson to the latest version
Change-Id: Iec76a7df76f8e1928828fd716c4c25f48e887ba1
You can merge this pull request into a Git repository by running:
$ git pull https
GitHub user phunt opened a pull request:
https://github.com/apache/zookeeper/pull/526
ZOOKEEPER-3050: update to the newest version of Jetty
Change-Id: I1f63c471e59a1ed9d1cf58e721fedf47452acc6b
You can merge this pull request into a Git repository by running:
$ git pull https
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/489
Can someone else take this on? Also it would be good to have some IPv6
tests included as well.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/525
These are useful fixes. However one of the burdens of recommending such a
fix is to also prove that the fix really addresses the issue, rather than just
moving the problem somewhere else. In
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/525#discussion_r189666458
--- Diff:
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -815,8 +815,11 @@ public void touchCnxn(NIOServerCnxn cnxn
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/524#discussion_r189664272
--- Diff: build.xml ---
@@ -55,8 +55,8 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyl
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/524
FYI this doesn't apply cleanly to branch-3.4, you'll have to submit a
separate PR if you want to fix that branch as well. (it does apply cleanly to
branch 3.5 though)
---
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/524#discussion_r189658262
--- Diff: build.xml ---
@@ -55,8 +55,8 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyl
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/524#discussion_r189658310
--- Diff: build.xml ---
@@ -55,8 +55,8 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyl
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/523#discussion_r189448291
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/StandaloneDisabledTest.java ---
@@ -179,6 +180,8 @@ private void startServer(int id, String
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/378
@Randgalt please close this manually - the PR has been merged. thx.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/444
Please close this - another PR was merged that includes this change. Thanks!
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/445
Please close this - another PR was merged that includes this change. Thanks!
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/520
+1, please close this PR manually
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/519
+1, please close this PR manually
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/462
given this is not default branch please close the PR manually
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/462
+1 lgtm.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/518
+1. Thanks @anmolnar please close this.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/517
+1. I committed to 3.5 and master however it doesn't apply cleanly to
branch-3.4. Please submit a separate PR.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/378
@Randgalt can you close this out? I applied the master PR to branch-3.5 and
committed it already. I think this is taken care of, lmk otw.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/516
+1 - thanks Andor.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/515
+1, thanks @anmolnar
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/514
+1, thanks @anmolnar
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/501
I'm afraid this is no longer auto-merging.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/509
+1, good catch. I committed this to all three active branches - please take
a look at 3.4 branch commit as I did need to resolve a conflict. (seemed minor
and tests are passing)
---
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/501#discussion_r184549466
--- Diff: src/contrib/monitoring/check_zookeeper.py ---
@@ -256,6 +261,36 @@ def _parse_stat(self, data):
result['zk_znode_
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/495
+1. Yes, it looks like those failures are unrelated (tests passed for me).
Thanks @lujiefsi and @brettKK
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/444
Does this need to be updated to reflect #443 ?
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/445
Does this need to be updated to reflect #443 ?
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/443
+1, lgtm. I've committed this to master. Thanks @mfenes !
---
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/496#discussion_r184218439
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java
---
@@ -94,7 +94,10 @@ public void authenticate(Socket sock
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/508
@anmolnar you will have to close this manually as it's not master branch.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/508
+1. Great. Thanks @anmolnar . I've committed for 3.4.13.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/501
I'm afraid this is no longer auto-merging, also the PR build failed on
jenkins with similar:
> Automatic merge failed; fix conflicts and then commit the result.
@nkalmar c
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/495
Ok, that (call site analysis) makes sense.
I'm afraid I was unclear, when I said
"You don't need an ERROR in the text here or on the next line."
Wha
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/495
Understood on throwing the exception (1&2). I'm interested in 3 - when it
is thrown is it handled correctly or some unexpected sideeffect. If we're going
to try to fix we should re
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/495
I ran out of time to answer this question, perhaps you can tell me - what
happens when the RTE is thrown? Is the caller handling it
appropriately/reasonably, or are we just pushing the problem
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/495#discussion_r183918644
--- Diff:
src/java/main/org/apache/zookeeper/server/ReferenceCountedACLCache.java ---
@@ -109,6 +109,10 @@ public synchronized void deserialize
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/469
Yes, please close it out. Thanks @mfenes , et. al.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/508
tbh would rather not. Given it's available here as a patch for anyone that
needs it why don't we just include it in 3.5+ . Anyone that wants it can apply
themselves and use it using w
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/506
Committed, but you'll need to close this manually @anmolnar given it's not
master.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/497
+1. lgtm thanks @anmolnar !
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/497
Created https://issues.apache.org/jira/browse/ZOOKEEPER-3026 for followup
on my ReadOnlyModeTest comment above.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/440
3.5 is still in beta, so I'm not super worried wrt it being an addition. I
also think it's super valuable to improve reporting. I'm erring on the side of
inclusion - any objections?
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/466
@anmolnar should we apply the same logic as my recent comment on #466 ? Add
this to jmx and the mntr command output, but don't add it to the legacy/text
4lw. What do you think.
---
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/501#discussion_r183907088
--- Diff: src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java
---
@@ -167,6 +167,8 @@ public void testValidateStatOutput() throws Exception
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/505
+1. Thanks @andschwa
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/469
@mfenes @anmolnar @afine - ant already has a -d and a -v option, why do
we need to add additional? Is it really necessary that all the builds include
this information? Why is -d/-v not sufficient?
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/501
Can we get this into 3.4 as well?
---
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/501#discussion_r183573019
--- Diff: src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java
---
@@ -167,6 +167,8 @@ public void testValidateStatOutput() throws Exception
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/501#discussion_r183571986
--- Diff: docs/zookeeperAdmin.html ---
@@ -2213,6 +2213,7 @@ The Four Letter Words
zk_min_latency 0
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/490
This is a good addition, thanks for bringing to our attention @leventov .
Unfortunately however we can't replace the std javac compiler with
error_prone in the toolchain outright - w
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/466#discussion_r183565978
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1897,6 +1896,12 @@ server.3=zoo3:2888:3888
zk_pending_syncs
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/466#discussion_r183565848
--- Diff: src/contrib/monitoring/ganglia/zookeeper_ganglia.py ---
@@ -213,7 +213,13 @@ def metric_init(params=None
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/466
is this intended for 3.4 or just 3.5+ ? The jira says 3.5+, is that right?
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/440
@anmolnar I believe you misunderstood my comment above:
> likely this change will go into master (3.6.0+) - impact on backports.
I'm assuming we would want to take advantage of this
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/497
+1. Looks reasonable however:
1) this problem also exists in
src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java - do you want to
fix it as part of this change or handle as a
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/487
+1 Thanks @anmolnar this looks good. Please consider backporting to 3.4
(separate jira).
Also in future please don't include any changed files from the toplevel
docs directory (htm
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/487
Looks promising - doesn't seem very useful (and potentially dangerous)
without docs - perhaps add a troubleshooting or recovery section here?
http://zookeeper.apache.org/doc/r3
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/497#discussion_r181526076
--- Diff: src/java/test/org/apache/zookeeper/test/SessionTimeoutTest.java
---
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/497#discussion_r181525743
--- Diff: src/java/test/org/apache/zookeeper/test/SessionTimeoutTest.java
---
@@ -0,0 +1,188 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r166787684
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -950,6 +952,39 @@ server.3=zoo3:2888:3888
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r166787489
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -950,6 +952,39 @@ server.3=zoo3:2888:3888
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163712641
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener
getZooKeeperServerListener
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/443#discussion_r163712079
--- Diff: build.xml ---
@@ -1406,50 +1410,53 @@
xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
+
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/377
@Randgalt i'd really like to push out a 3.5.4 - do you think it would make
sense to release note this in 3.5.4 and address for 3.5.5? If you think we can
finalize this PR soon I'm sti
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163420180
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener
getZooKeeperServerListener
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163419925
--- Diff: src/java/main/org/apache/zookeeper/server/EphemeralType.java ---
@@ -40,19 +39,40 @@
TTL;
public static final long
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/440
No apologies necessary @aberghage - appreciate the feedback.
---
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/440
I like the idea of moving to dropwizard vs our own homebrew. Seems like it
would allow us richer insight for metrics. Here are my concerns:
1) performance. Do we know the impact of this
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/440#discussion_r163391127
--- Diff: src/java/main/org/apache/zookeeper/server/ZKDatabase.java ---
@@ -264,19 +262,8 @@ public void addCommittedProposal(Request request
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/443#discussion_r163363116
--- Diff: build.xml ---
@@ -1406,50 +1410,53 @@
xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
+
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/446#discussion_r163063447
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
---
@@ -1751,7 +1751,7 @@ public synchronized void initConfigInZKDatabase
1 - 100 of 244 matches
Mail list logo
