[GitHub] zookeeper issue #566: ZOOKEEPER-3062: mention fsync.warningthresholdms in Fi...
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] zookeeper issue #47: Update zookeeperOver.html
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/47 @cpoerschke please close this given your recent comment. thanks. ---
[GitHub] zookeeper issue #350: fix comment type error.
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] zookeeper issue #575: ZOOKEEPER-3093 sync zerror with ZOO_ERRORS
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] zookeeper issue #557: ZOOKEEPER-3077: build outside of source directory
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] zookeeper issue #576: ZOOKEEPER-3046: increased test timeout
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] zookeeper issue #466: ZOOKEEPER-2940. Deal with maxbuffer as it relates to l...
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] zookeeper issue #466: ZOOKEEPER-2940. Deal with maxbuffer as it relates to l...
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] zookeeper issue #528: ZOOKEEPER-3034 Facing issues while building from sourc...
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] zookeeper issue #528: ZOOKEEPER-3034 Facing issues while building from sourc...
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] zookeeper issue #528: ZOOKEEPER-3034 Facing issues while building from sourc...
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 it - it's a new dependency. Here's an example of some pushback in the curl community https://github.com/curl/curl/issues/972 If you do want to add this you probably need to add something like: ``` m4_ifndef([PKG_PROG_PKG_CONFIG], [m4_fatal([Could not locate the pkg-config autoconf macros. Please make sure pkg-config is installed and, if necessary, set the environment variable ACLOCAL="aclocal -I/path/to/pkg.m4".])]) ``` although I'm honestly not very psyched about this change. It's going to break a bunch of folk's builds. ---
[GitHub] zookeeper issue #467: ZOOKEEPER-2968: Add C client code coverage tests
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 had to install lcov in order to run this. It would be a good idea to update the docs - probably the c README - with details on how to run coverage and what the requirements are. 3) What about the cmake build? Do we need to do anything there? (if not perhaps some docs?) ---
[GitHub] zookeeper issue #528: ZOOKEEPER-3034 Facing issues while building from sourc...
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 perl... /usr/bin/perl checking for dot... no checking for a BSD-compatible install... /usr/bin/install -c checking whether build environment is sane... yes checking for a thread-safe mkdir -p... /bin/mkdir -p checking for gawk... no checking for mawk... mawk checking whether make sets $(MAKE)... yes checking whether make supports nested variables... yes ./configure: line 5034: PKG_PROG_PKG_CONFIG: command not found ./configure: line 5039: syntax error near unexpected token `CPPUNIT,' ./configure: line 5039: `PKG_CHECK_MODULES(CPPUNIT, cppunit >= 1.10.2, , , )' I also see all kinds of warnings popping up: # autoreconf -if configure.ac:39: warning: PKG_PROG_PKG_CONFIG is m4_require'd but not m4_defun'd configure.ac:28: ZK_PKG_CONFIG_STATIC is expanded from... ../../lib/m4sugar/m4sh.m4:639: AS_IF is expanded from... configure.ac:39: the top level configure.ac:39: warning: PKG_PROG_PKG_CONFIG is m4_require'd but not m4_defun'd configure.ac:28: ZK_PKG_CONFIG_STATIC is expanded from... ../../lib/m4sugar/m4sh.m4:639: AS_IF is expanded from... configure.ac:39: the top level libtoolize: putting auxiliary files in `.'. libtoolize: copying file `./ltmain.sh' libtoolize: Consider adding `AC_CONFIG_MACRO_DIR([m4])' to configure.ac and libtoolize: rerunning libtoolize, to keep the correct libtool macros in-tree. libtoolize: Consider adding `-I m4' to ACLOCAL_AMFLAGS in Makefile.am. ---
[GitHub] zookeeper pull request #537: ZOOKEEPER-2920: Upgrade OWASP Dependency Check ...
Github user phunt closed the pull request at: https://github.com/apache/zookeeper/pull/537 ---
[GitHub] zookeeper issue #537: ZOOKEEPER-2920: Upgrade OWASP Dependency Check to 3.2....
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] zookeeper issue #536: ZOOKEEPER-2920: Upgrade OWASP Dependency Check to 3.2....
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] zookeeper pull request #537: ZOOKEEPER-2920: Upgrade OWASP Dependency Check ...
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 d065898f77c9162faaffb9fa0dad543eb07771db) You can merge this pull request into a Git repository by running: $ git pull https://github.com/phunt/zookeeper ZOOKEEPER-2920-br34 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/537.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 #537 commit 37e4015ff07a96ce66dd8a3cf6c6df2004119d64 Author: Patrick Hunt Date: 2018-05-31T00:49:03Z 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 d065898f77c9162faaffb9fa0dad543eb07771db) ---
[GitHub] zookeeper pull request #536: ZOOKEEPER-2920: Upgrade OWASP Dependency Check ...
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 request into a Git repository by running: $ git pull https://github.com/phunt/zookeeper ZOOKEEPER-2920 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/536.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 #536 commit d065898f77c9162faaffb9fa0dad543eb07771db Author: Patrick Hunt Date: 2018-05-31T00:49:03Z ZOOKEEPER-2920: Upgrade OWASP Dependency Check to 3.2.1 Updated and verified on all the active branches. Change-Id: I750d7e576e8c731aab4cef26f6863eac6b1b8dc2 ---
[GitHub] zookeeper issue #531: [ZOOKEEPER-2317] Ensure that OSGi versions are valid
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/531 +1, lgtm. Thanks @timothyjward ! ---
[GitHub] zookeeper issue #532: ZOOKEEPER-3043: QuorumKerberosHostBasedAuthTest fails ...
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] zookeeper issue #524: ZOOKEEPER-3043: QuorumKerberosHostBasedAuthTest fails ...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/524 +1, lgtm. Thanks @eolivelli ---
[GitHub] zookeeper issue #528: ZOOKEEPER-3034 Facing issues while building from sourc...
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 container directly to aid in testing. these images have cppunit installed - you can use the package manager to uninstall them if you want to test with and without --without-cppunit ---
[GitHub] zookeeper issue #528: ZOOKEEPER-3034 Facing issues while building from sourc...
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 perl... /usr/bin/perl checking for dot... no checking for a BSD-compatible install... /usr/bin/install -c checking whether build environment is sane... yes checking for a thread-safe mkdir -p... /bin/mkdir -p checking for gawk... no checking for mawk... mawk checking whether make sets $(MAKE)... yes checking whether make supports nested variables... yes ./configure: line 5034: syntax error near unexpected token `CPPUNIT,' ./configure: line 5034: `PKG_CHECK_MODULES_STATIC(CPPUNIT, cppunit >= 1.10.2)' ---
[GitHub] zookeeper issue #511: Incorrect constant value
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] zookeeper issue #522: ZOOKEEPER-1919 Update the C implementation of removeWa...
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 issue regardless: 1) tests/zkServer.sh isn't sleeping long enough and the client gives up too soon (RO mode takes a while to kick in) 2) IPV6 is identified but failing 3) remove watches wasn't added to the c cli so you can't exercise outside the test. I'll enter jiras for these. Some are good Newbie issues. ---
[GitHub] zookeeper issue #524: ZOOKEEPER-3043: QuorumKerberosHostBasedAuthTest fails ...
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] zookeeper issue #527: ZOOKEEPER-3051: Updated jackson to the latest version
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/527 Done. Thanks @hanm ---
[GitHub] zookeeper pull request #527: ZOOKEEPER-3051: Updated jackson to the latest v...
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.com/phunt/zookeeper ZOOKEEPER-3051 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/527.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 #527 commit 2af2138d3ae334bf5f0a6673caf42ae55174ddea Author: Patrick Hunt <phunt@...> Date: 2018-05-21T19:55:54Z ZOOKEEPER-3051: Updated jackson to the latest version Change-Id: Iec76a7df76f8e1928828fd716c4c25f48e887ba1 ---
[GitHub] zookeeper pull request #526: ZOOKEEPER-3050: update to the newest version of...
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.com/phunt/zookeeper ZOOKEEPER-3050 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/526.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 #526 commit b0f2291056284c72c8bb84f0c0d9d10a88cd3ced Author: Patrick Hunt <phunt@...> Date: 2018-05-21T19:26:34Z ZOOKEEPER-3050: update to the newest version of Jetty Change-Id: I1f63c471e59a1ed9d1cf58e721fedf47452acc6b ---
[GitHub] zookeeper issue #489: ZOOKEEPER-2989: IPv6 literal address causes problems f...
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] zookeeper issue #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxnFactory
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 particular it would be good if the description of this fix (and any such fix) included details on whether all of the callers to addCnxn handle the new error handling code properly. In this case throwing the IOException in addCnxn. "some of their callers have !=null check but some do not have" One of the things any committer will need to do is also verify, and having this thought out ahead of time will help. Another thing - are the null checks in the caller code still necessary? Should they be removed? If yes any such patch should include changes there as well. ---
[GitHub] zookeeper pull request #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxn...
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) {
cnxnExpiryQueue.update(cnxn, cnxn.getSessionTimeout());
}
-private void addCnxn(NIOServerCnxn cnxn) {
+private void addCnxn(NIOServerCnxn cnxn) throws IOException {
InetAddress addr = cnxn.getSocketAddress();
+if (addr == null) {
+throw new IOException("Scoket of " + cnxn + " has been
closed");
--- End diff --
fix the spelling.
---
[GitHub] zookeeper pull request #524: ZOOKEEPER-3043: QuorumKerberosHostBasedAuthTest...
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.checkstyle.ant"> - - + + --- End diff -- Changing to 1.1.0 worked for me - it compiled and the tests ran. Is there something else? I did try 1.1.1 but that had the issue you mentioned. ---
[GitHub] zookeeper issue #524: ZOOKEEPER-3043: QuorumKerberosHostBasedAuthTest fails ...
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] zookeeper pull request #524: ZOOKEEPER-3043: QuorumKerberosHostBasedAuthTest...
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.checkstyle.ant"> - - + --- End diff -- 2.6 is available - why not update to that? ---
[GitHub] zookeeper pull request #524: ZOOKEEPER-3043: QuorumKerberosHostBasedAuthTest...
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.checkstyle.ant"> - - + + --- End diff -- 1.1.0 is available - why not update to that? ---
[GitHub] zookeeper pull request #523: ZOOKEEPER-3046: added junit timeout to precede ...
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 config) throws
Exception {
zkHandles[id] = ClientBase.createZKClient("127.0.0.1:" +
clientPorts[id]);
zkAdminHandles[id] = new ZooKeeperAdmin("127.0.0.1:" +
clientPorts[id], CONNECTION_TIMEOUT, this);
zkAdminHandles[id].addAuthInfo("digest", "super:test".getBytes());
+LOG.info(String.format("Started server id %d with
config:\n%s\nStat output:\n%s",
+id, config,
FourLetterWordMain.send4LetterWord("127.0.0.1", clientPorts[id], "stat")));
--- End diff --
Can you separate out the 4lw call onto it's own line?
---
[GitHub] zookeeper issue #378: [ZOOKEEPER-2903] Backport of ZOOKEEPER-2901 changes
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] zookeeper issue #444: ZOOKEEPER-2955: Enable Clover code coverage report
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] zookeeper issue #445: ZOOKEEPER-2955: Enable Clover code coverage report
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] zookeeper issue #520: ZOOKEEPER-2955: Enable Clover code coverage report
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/520 +1, please close this PR manually ---
[GitHub] zookeeper issue #519: ZOOKEEPER-2955: Enable Clover code coverage report
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/519 +1, please close this PR manually ---
[GitHub] zookeeper issue #462: ZOOKEEPER-2980 Backport ZOOKEEPER-2939 Deal with maxbu...
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] zookeeper issue #462: ZOOKEEPER-2980 Backport ZOOKEEPER-2939 Deal with maxbu...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/462 +1 lgtm. ---
[GitHub] zookeeper issue #518: ZOOKEEPER-3039 TxnLogToolkit uses Scanner badly (3.4)
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/518 +1. Thanks @anmolnar please close this. ---
[GitHub] zookeeper issue #517: ZOOKEEPER-3039 TxnLogToolkit uses Scanner badly
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] zookeeper issue #378: [ZOOKEEPER-2903] Backport of ZOOKEEPER-2901 changes
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] zookeeper issue #516: ZOOKEEPER-3038 Cleanup some nitpicks in TTL implementa...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/516 +1 - thanks Andor. ---
[GitHub] zookeeper issue #515: ZOOKEEPER-3012. Fix unit test: testDataDirAndDataLogDi...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/515 +1, thanks @anmolnar ---
[GitHub] zookeeper issue #514: ZOOKEEPER-3012. Fix unit test: testDataDirAndDataLogDi...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/514 +1, thanks @anmolnar ---
[GitHub] zookeeper issue #501: ZOOKEEPER-3019 add metric for slow fsyncs count
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/501 I'm afraid this is no longer auto-merging. ---
[GitHub] zookeeper issue #509: ZOOKEEPER-3027 Accidently removed public API of FileTx...
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] zookeeper pull request #501: ZOOKEEPER-3019 add metric for slow fsyncs count
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_count'] = int(m.group(1))
continue
+m = re.match('Watch count: (\d+)', line)
+if m is not None:
+result['zk_watch_count'] = int(m.group(1))
+continue
+
+m = re.match('Ephemerals count: (\d+)', line)
+if m is not None:
+result['zk_ephemerals_count'] = int(m.group(1))
+continue
+
+m = re.match('Approximate data size: (\d+)', line)
+if m is not None:
+result['zk_approximate_data_size'] = int(m.group(1))
+continue
+
+m = re.match('Open file descriptor count: (\d+)', line)
+if m is not None:
+result['zk_open_file_descriptor_count'] = int(m.group(1))
+continue
+
+m = re.match('Max file descriptor count: (\d+)', line)
+if m is not None:
+result['zk_max_file_descriptor_count'] = int(m.group(1))
+continue
+
+m = re.match('Fsync threshold exceeded: (\d+)', line)
--- End diff --
You might need to check the supporting scripts - I don't believe this is
output any longer, right?
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
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] zookeeper issue #444: ZOOKEEPER-2955: Enable Clover code coverage report
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/444 Does this need to be updated to reflect #443 ? ---
[GitHub] zookeeper issue #445: ZOOKEEPER-2955: Enable Clover code coverage report
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/445 Does this need to be updated to reflect #443 ? ---
[GitHub] zookeeper issue #443: ZOOKEEPER-2955: Enable Clover code coverage report
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] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
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, String hostName)
throws IOException {
principalConfig,
QuorumAuth.QUORUM_SERVER_PROTOCOL_NAME,
QuorumAuth.QUORUM_SERVER_SASL_DIGEST, LOG,
"QuorumLearner");
-
+if (sc == null) {
--- End diff --
Same feedback as #495
1) check the callers and see if it's handled properly. Likely it will be
logged there as well. Verify/report.
2) No need to say exception in an exception. The text of LOG.error line
seems like it would have been a good error string for the exception itself.
3) as previously noted, add a test.
---
[GitHub] zookeeper issue #508: ZOOKEEPER-2994 Tool required to recover log and snapsh...
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] zookeeper issue #508: ZOOKEEPER-2994 Tool required to recover log and snapsh...
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] zookeeper issue #501: ZOOKEEPER-3019 add metric for slow fsyncs count
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 could you rebase this on latest master? Thanks. ---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
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."
What I mean is that the text string should not start with "ERROR" given the
error string is in an exception and the logging (from one of the callers) will
determine the severity to assign. As such my recommendation would be something
like:
> throw new RuntimeException("Incorrect format of InputArchive when
deserialize DataTree - missing acls");
Notice: 1) the removal of "ERROR" and the addition of "missing acls" in
order to give the person diagnosing the problem a bit more insight (otw they
have to find the source line in order to get more insight into what they
formatting issue might be).
If you clear this up (this one line) I think we should be good for commit.
Thanks!
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
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 really ensure we fix it. ---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
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 somewhere else? ---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
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(InputArchive ia)
throws IOException {
}
List aclList = new ArrayList();
Index j = ia.startVector("acls");
+if (j == null) {
+LOG.error("ERROR: incorrent format of InputArchive" + ia);
--- End diff --
You don't need an ERROR in the text here or on the next line. It's already
LOG'd as an error. Same for the next line - it's a RTE.
Also you need a space "... of InputArchive" -> " of InputArchive "
(notice space at the end. Otw the text of ia is just appended w/o the space.
Also notice that ia doesn't have a toString, so I'm not sure how helpful that
is it's fine to leave I guess.
---
[GitHub] zookeeper issue #469: ZOOKEEPER-2983: Print the classpath when running compi...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/469 Yes, please close it out. Thanks @mfenes , et. al. ---
[GitHub] zookeeper issue #508: ZOOKEEPER-2994 Tool required to recover log and snapsh...
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 what you've provided here. Make sense @anmolnar ? If so close this. Otw lmk. ---
[GitHub] zookeeper issue #506: ZOOKEEPER-2415 SessionTest is using Thread deprecated ...
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] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/497 +1. lgtm thanks @anmolnar ! ---
[GitHub] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
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] zookeeper issue #440: ZOOKEEPER-2979 Use dropwizard library histogram for pr...
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] zookeeper issue #466: ZOOKEEPER-2940. Deal with maxbuffer as it relates to l...
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] zookeeper pull request #501: ZOOKEEPER-3019 add metric for slow fsyncs count
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 {
line = in.readLine();
Assert.assertTrue(Pattern.matches("^Mode: .*$", line));
line = in.readLine();
+Assert.assertTrue(Pattern.matches("^Fsync threshold exceeded:
\\d+$", line));
--- End diff --
That's pretty ugly, esp. if there are changes in the future.
Given the Monitor command is available since 3.4.0 and handles (documented)
such changes correctly perhaps we should just add this to monitor and not add
it to stat. If a user wants the additional information they can use mntr
instead.
What do you think @nkalmar ?
The other benefit is that it will integrate into 3.4 more easily - i.e.
fully b/w compat w/o any worries.
---
[GitHub] zookeeper issue #505: ZOOKEEPER-3025: Make `hashtable` search `include`
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/505 +1. Thanks @andschwa ---
[GitHub] zookeeper issue #469: ZOOKEEPER-2983: Print the classpath when running compi...
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] zookeeper issue #501: ZOOKEEPER-3019 add metric for slow fsyncs count
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/501 Can we get this into 3.4 as well? ---
[GitHub] zookeeper pull request #501: ZOOKEEPER-3019 add metric for slow fsyncs count
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 {
line = in.readLine();
Assert.assertTrue(Pattern.matches("^Mode: .*$", line));
line = in.readLine();
+Assert.assertTrue(Pattern.matches("^Fsync threshold exceeded:
\\d+$", line));
--- End diff --
any chance we can add this to the very end of the output? We weren't very
clear on order and in some cases existing monitoring may be expecting the
original order. Appending helps wrt b/w compat.
---
[GitHub] zookeeper pull request #501: ZOOKEEPER-3019 add metric for slow fsyncs count
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
zk_packets_received 70
zk_packets_sent 69
+ zk_num_alive_connections 1
--- End diff --
Please remove doc/{html,pdf} from PRs - these files are regenerated during
commit time by the committer. (and are a pain to manage wrt the PR merge tool)
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
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 - we can have a separate option/target to do this, but it can't be the default. I would recommend separating this into two jiras/pullrequests - one to change the build, the other to address the found issues. @leventov you mentioned "I'm not proficient in Ant" - perhaps one of the other folks could help with this? @anmolnar you mentioned hbase is using it - anything we can pull from there wrt how they integrate it into their build? ps. I like the idea of also getting this into 3.4 branch as well, which means it needs to be optional so that the user can run it when they have java8+, but otw not. Which matches my intent/statement above. pps. looks like there is a new version of error_prone, might want to take this opportunity to update the dependency version. I notice the library is ASL2.0 which is also good (license is in the COPYING file currently) ---
[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...
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_syncs0 - only exposed by the Leader zk_open_file_descriptor_count 23- only available on Unix platforms zk_max_file_descriptor_count 1024 - only available on Unix platforms + zk_last_proposal_size --- End diff -- these are missing example values ---
[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...
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):
'zk_max_file_descriptor_count': {'units': 'descriptors'},
'zk_followers': {'units': 'nodes'},
'zk_synced_followers': {'units': 'nodes'},
-'zk_pending_syncs': {'units': 'syncs'}
+'zk_pending_syncs': {'units': 'syncs'},
+'zk_last_proposal_size': {'units': 'ms'},
--- End diff --
Should these values be in "ms"? Seems like a cut/paste typo? I suspect you
rather "bytes" - right?
---
[GitHub] zookeeper issue #466: ZOOKEEPER-2940. Deal with maxbuffer as it relates to l...
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] zookeeper issue #440: ZOOKEEPER-2979 Use dropwizard library histogram for pr...
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 library across the board. (can we do that? I assume yes?) > Sure, I'll have to backport it to branch-3.4, it will need some manual work for sure, but I don't expect it to be the end of the world. I meant that I was planning to only commit this change for 3.6.0+ - I am generally reticent to add a new dependency so late in the 3.5 train. Do you feel strongly otw? ---
[GitHub] zookeeper issue #497: ZOOKEEPER-2415. SessionTest is using Thread deprecated...
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 separate jira? (either way is fine with me - separate might make it simpler to review/commit and make progress tbh, but up to you) 2) regardless 1) above, this doesn't apply to branch-3.4 - the original jira says it affects 3.4, I'm assuming we want to fix there as well? Could you create a separate PR for this? I'd like to commit both at the same time. ---
[GitHub] zookeeper issue #487: ZOOKEEPER-2994 Tool required to recover log and snapsh...
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 (html/pdf files) as these are regenerated during commit. ---
[GitHub] zookeeper issue #487: ZOOKEEPER-2994 Tool required to recover log and snapsh...
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.4.11/zookeeperAdmin.html#sc_dataFileManagement The jira was original for 3.5+, I think this would be great to get into 3.4+. ---
[GitHub] zookeeper pull request #497: ZOOKEEPER-2415. SessionTest is using Thread dep...
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 (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.DisconnectableZooKeeper;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+public class SessionTimeoutTest extends ZKTestCase {
--- End diff --
Is there a reason why we can't use ClientBase instead of ZKTestCase?
---
[GitHub] zookeeper pull request #497: ZOOKEEPER-2415. SessionTest is using Thread dep...
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 (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.DisconnectableZooKeeper;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+public class SessionTimeoutTest extends ZKTestCase {
+protected static final Logger LOG =
LoggerFactory.getLogger(SessionTest.class);
--- End diff --
Should be SessionTimeoutTest rather than SessionTest.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
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 + +zookeeper.extendedTypesEnabled + + +(Java system property only: zookeeper.extendedTypesEnabled) + + New in 3.5.4, 3.6.0: Define to "true" to enable + extended features such as the creation of TTL Nodes. + They are disabled by default. IMPORTANT: when enabled server IDs must + be less than 255 due to internal limitations. + + + + + +zookeeper.emulate353TTLNodes + + +(Java system property only: zookeeper.emulate353TTLNodes) + + New in 3.5.4, 3.6.0: Due to +https://issues.apache.org/jira/browse/ZOOKEEPER-2901;>ZOOKEEPER-2901 TTL nodes +created in version 3.5.3 are not supported in 3.5.4/3.6.0. However, a workaround is provided via the +zookeeper.emulate353TTLNodes system property. If you used TTL nodes in ZooKeeper 3.5.3 and need to maintain +compatibility set zookeeper.emulate353TTLNodes to "true" in addition to +zookeeper.extendedTypesEnabled. NOTE: due to the bug, server IDs +must be 127 or less. Additionally, the maximum support TTL value is 1099511627775 which is smaller +than what was allowed in 3.5.3 (1152921504606846975) --- End diff -- Is there documentation around this? I'd recommend documenting the bounds. ---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
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 + +zookeeper.extendedTypesEnabled + + +(Java system property only: zookeeper.extendedTypesEnabled) + + New in 3.5.4, 3.6.0: Define to "true" to enable + extended features such as the creation of TTL Nodes. + They are disabled by default. IMPORTANT: when enabled server IDs must + be less than 255 due to internal limitations. + + + + + +zookeeper.emulate353TTLNodes --- End diff -- What's the plan for deprecating this? Keeping this parameter around forever seems like a bad idea. Adds complication that we don't really need to carry around. Perhaps we can deprecate in 3.5.4 and remove in 3.5.5? Similarly I don't think we should include this at all in 3.6.0 (trunk). ---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
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() {
return listener;
}
+// Visible for testing
+static volatile int serverId = 1;
--- End diff --
It being a global seems to kick the problem down the road to the next
person. I'm worried that setting this jvm wide could have an impact when
developing tests in the future. That's why it caught my eye.
---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
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"> + --- End diff -- "I think declaring a "test-coverage-java" target to run Clover does not mean "coverage" == clover. The fact that it currently calls "test-core-java" and "generate-clover-reports" does not mean that we would not allow anybody to add/use other code coverage tools here." Notice how we have cobertura down below in the build.xml file. This is what I mean. "test-coverage-java" just calls clover. If we had created a "test-coverage-java" in the past that called cobertura it seems like that's a statement. It's not a big deal, but hopefully you see what I mean. Renaming to be more clover specific seems reasonable to me. ---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
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 still open to that. ---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
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() {
return listener;
}
+// Visible for testing
+static volatile int serverId = 1;
--- End diff --
I looked at it a bit originally, iirc I figured that you'd pass it through
in some way (during instance creation) rather than making it a global.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
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 CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
-public static final long MAX_TTL = 0x0fffL;
-public static final long TTL_MASK = 0x8000L;
+public static final long MAX_TTL = 0x00ffL;
+public static final long TTL_MASK = 0xff00L;
+public static final long MAX_TTL_SERVER_ID = 0xfe; // 254
+
+public static final String EXTENDED_TYPES_ENABLED_PROPERTY =
"zookeeper.extendedTypesEnabled";
+
+public static boolean extendedEphemeralTypesEnabled() {
+return Boolean.getBoolean(EXTENDED_TYPES_ENABLED_PROPERTY);
+}
public static EphemeralType get(long ephemeralOwner) {
+if ( extendedEphemeralTypesEnabled() ) {
+if ((ephemeralOwner & TTL_MASK) == TTL_MASK) {
--- End diff --
We discussed this earlier in the PR. Problem is it's b/w incompat one way
or the other. Given serverid has been around forever, and TTL only added in
3.5.3 (a beta) we went forward with this approach.
---
[GitHub] zookeeper issue #440: ZOOKEEPER-2939 Deal with maxbuffer as it relates to pr...
Github user phunt commented on the issue: https://github.com/apache/zookeeper/pull/440 No apologies necessary @aberghage - appreciate the feedback. ---
[GitHub] zookeeper issue #440: ZOOKEEPER-2939 Deal with maxbuffer as it relates to pr...
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 library vs our homebrew? Both in the small and the large. 2) likely this change will go into master (3.6.0+) - impact on backports. I'm assuming we would want to take advantage of this library across the board. (can we do that? I assume yes?) 3) I believe I recommended on the original PR separating the new metric from introducing new dependencies. I still believe that's the quickest way to get the feature addressed, vs introducing new metrics functionality. Can we commit #415 independently of this change? ---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
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) {
maxCommittedLog = request.zxid;
}
-ByteArrayOutputStream baos = new ByteArrayOutputStream();
-BinaryOutputArchive boa = BinaryOutputArchive.getArchive(baos);
-try {
-request.getHdr().serialize(boa, "hdr");
-if (request.getTxn() != null) {
-request.getTxn().serialize(boa, "txn");
-}
-baos.close();
-} catch (IOException e) {
-LOG.error("This really should be impossible", e);
-}
-QuorumPacket pp = new QuorumPacket(Leader.PROPOSAL,
request.zxid,
-baos.toByteArray(), null);
+byte[] data = SerializeUtils.serializeRequest(request);
--- End diff --
Is this really apropos to the stated reason for the PR? If not separating
out to another PR means 1) easier to review this patch, and 2) might be easier
to get this committed separately rather than tying it to another issue.
---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
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"> + --- End diff -- Why do we need this given we have a "clover" target? I'm also hesitant to explicitly code that "coverage" == clover. In the past we've allowed coverage other than clover to be used. perhaps "test-coverage-clover-java" instead? ---
[GitHub] zookeeper pull request #446: ZOOKEEPER-1580:QuorumPeer.setRunning is not use...
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() {
if (zkDb != null) zkDb.initConfigInZKDatabase(getQuorumVerifier());
}
-public void setRunning(boolean running) {
+private void setRunning(boolean running) {
--- End diff --
@maoling ?
---
