[GitHub] zookeeper pull request #471: ZOOKEEPER-2983: Print the classpath when runnin...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/471 ---
[GitHub] zookeeper pull request #470: ZOOKEEPER-2983: Print the classpath when runnin...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/470 ---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/469 ---
[GitHub] zookeeper issue #469: ZOOKEEPER-2983: Print the classpath when running compi...
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/469 @phunt @anmolnar Should I close this PR then? ---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/469#discussion_r179420191 --- Diff: build.xml --- @@ -1861,4 +1861,18 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- @afine ok, I've replaced "_" with "-" in the new ant target names. ---
[GitHub] zookeeper pull request #391: ZOOKEEPER-2727: WARN and stacktrace for normall...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/391 ---
[GitHub] zookeeper pull request #375: ZOOKEEPER-1363: Categorise unit tests by 'test-...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/375 ---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/490 Hi @leventov, in the commits, there are test code changes, too. Are these related to the error prone compiler change, or these are just other bugs which you've fixed? ---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175393694
--- Diff: build.xml ---
@@ -475,23 +484,36 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
pattern="${ivy.lib}/[artifact]-[revision].[ext]"/>
-
-
--- End diff --
Could we add the error prone compiler as an optional tool, so that
similarly to e.g. code coverage tools, the build could be run with or without
the tool?
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175393897
--- Diff: build.xml ---
@@ -475,23 +484,36 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
pattern="${ivy.lib}/[artifact]-[revision].[ext]"/>
-
-
+
+
+
+
+
-
+
--- End diff --
Could we add the error prone compiler as an optional tool, so that
similarly to e.g. code coverage tools, the build could be run with or without
the tool?
---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/469#discussion_r169695831 --- Diff: build.xml --- @@ -1861,4 +1861,18 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- True, my intention with the "_" was to indicate that these targets are purely utility targets i.e. it does not make too much sense to run them individually, unlike clean, compile, test-core-java etc. targets. If you don't like this idea, I can replace the "_"-s with "-"-s if you wish. ---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/469#discussion_r169691663 --- Diff: build.xml --- @@ -1861,4 +1861,18 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + + --- End diff -- The difference between the two is that the one I used prints the paths in a pretty printed way, while the "one-liner" prints all paths continuously which is hard to read. ---
[GitHub] zookeeper pull request #459: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/459 ---
[GitHub] zookeeper pull request #458: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/458 ---
[GitHub] zookeeper pull request #471: ZOOKEEPER-2983: Print the classpath when runnin...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/471 ZOOKEEPER-2983: Print the classpath when running compile and test ant targets ZOOKEEPER-2983: Print the classpath when running compile and test ant targets I've added 2 new ant targets to print the compile and test classpath in a formatted way. Printing the classpath helps to verify that we have only the intended classes, jars on the classpath, e.g. clover.jar is included only when running coverage tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2983_3.4 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/471.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 #471 commit 56ac30cd88744f8b0bc850235263ad627804c36c Author: Mark Fenes <mfenes@...> Date: 2018-02-20T16:07:46Z ZOOKEEPER-2983: Print the classpath when running compile and test ant targets ---
[GitHub] zookeeper pull request #470: ZOOKEEPER-2983: Print the classpath when runnin...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/470 ZOOKEEPER-2983: Print the classpath when running compile and test ant targets ZOOKEEPER-2983: Print the classpath when running compile and test ant targets I've added 2 new ant targets to print the compile and test classpath in a formatted way. Printing the classpath helps to verify that we have only the intended classes, jars on the classpath, e.g. clover.jar is included only when running coverage tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2983_3.5 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/470.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 #470 commit 4e3b2d16be7f58064ff73206d96f5fe477c3d3fa Author: Mark Fenes <mfenes@...> Date: 2018-02-20T15:58:04Z ZOOKEEPER-2983: Print the classpath when running compile and test ant targets ---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/469 ZOOKEEPER-2983: Print the classpath when running compile and test ant targets ZOOKEEPER-2983: Print the classpath when running compile and test ant targets I've added 2 new ant targets to print the compile and test classpath in a formatted way. Printing the classpath helps to verify that we have only the intended classes, jars on the classpath, e.g. clover.jar is included only when running coverage tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2983 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/469.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 #469 commit 44b1f07cd129bc1f77fcb27bb509d9890d10207f Author: Mark Fenes <mfenes@...> Date: 2018-02-20T14:12:52Z ZOOKEEPER-2983: Print the classpath when running compile and test ant targets ---
[GitHub] zookeeper pull request #467: ZOOKEEPER-2968: Add C client code coverage test...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/467 ZOOKEEPER-2968: Add C client code coverage tests ZOOKEEPER-2968: Add C client code coverage tests This PR adds a new ant target 'c_coverage_report' which generates coverage report for the ZK C client. The report is generated to build/c_coverage/report in html format. As a requirement, lcov has to be installed prior to running target 'c_coverage_report'. An additional check was added to 'check-cppunit-makefile' to ensure that the Makefile gets deleted and regenerated without the coverage compiler flags when running targets without --enable-gcov. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2968 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/467.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 #467 commit 3c902262217c236cab2804a2932c7eb68f6f1539 Author: Mark Fenes <mfenes@...> Date: 2018-02-15T15:24:53Z ZOOKEEPER-2968: Add C client code coverage tests commit d8757821add5930b08bce52a11ddd7b3428d01ff Author: Mark Fenes <mfenes@...> Date: 2018-02-19T13:44:14Z ZOOKEEPER-2968: Add C client code coverage tests Added check to ensure that the Makefile gets regenerated if someone runs the cpp unit tests after generating C coverage reports without running ant clean first. ---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r168891236
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -35,92 +38,181 @@
public class FileTxnSnapLogTest {
-/**
- * Test verifies the auto creation of data dir and data log dir.
- * Sets "zookeeper.datadir.autocreate" to true.
- */
-@Test
-public void testWithAutoCreateDataLogDir() throws IOException {
-File tmpDir = ClientBase.createEmptyTestDir();
-File dataDir = new File(tmpDir, "data");
-File snapDir = new File(tmpDir, "data_txnlog");
-Assert.assertFalse("data directory already exists",
dataDir.exists());
-Assert.assertFalse("snapshot directory already exists",
snapDir.exists());
+private File tmpDir;
+
+private File logDir;
+
+private File snapDir;
+
+private File logVersionDir;
+
+private File snapVersionDir;
+
+@Before
+public void setUp() throws Exception {
+tmpDir = ClientBase.createEmptyTestDir();
--- End diff --
@eolivelli Thanks for the idea. I'll check and use this next time.
---
[GitHub] zookeeper pull request #459: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/459#discussion_r168889616
--- Diff: src/java/test/org/apache/zookeeper/test/ClientBase.java ---
@@ -351,20 +351,37 @@ static void verifyThreadTerminated(Thread thread,
long millis)
}
}
+public static File createEmptyTestDir() throws IOException {
+return createTmpDir(BASETEST, false);
+}
public static File createTmpDir() throws IOException {
-return createTmpDir(BASETEST);
+return createTmpDir(BASETEST, true);
}
-static File createTmpDir(File parentDir) throws IOException {
+
+static File createTmpDir(File parentDir, boolean createInitFile)
throws IOException {
File tmpFile = File.createTempFile("test", ".junit", parentDir);
// don't delete tmpFile - this ensures we don't attempt to create
// a tmpDir with a duplicate name
File tmpDir = new File(tmpFile + ".dir");
Assert.assertFalse(tmpDir.exists()); // never true if tmpfile does
it's job
Assert.assertTrue(tmpDir.mkdirs());
+// todo not every tmp directory needs this file
--- End diff --
Unfortunately this todo comment is in the master branch from where I
backported these changes in ClientBase.
---
[GitHub] zookeeper pull request #458: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/458#discussion_r16396
--- Diff: src/java/test/org/apache/zookeeper/test/ClientBase.java ---
@@ -368,22 +368,37 @@ static void verifyThreadTerminated(Thread thread,
long millis)
}
}
+public static File createEmptyTestDir() throws IOException {
+return createTmpDir(BASETEST, false);
+}
public static File createTmpDir() throws IOException {
-return createTmpDir(BASETEST);
+return createTmpDir(BASETEST, true);
}
-static File createTmpDir(File parentDir) throws IOException {
+static File createTmpDir(File parentDir, boolean createInitFile)
throws IOException {
File tmpFile = File.createTempFile("test", ".junit", parentDir);
// don't delete tmpFile - this ensures we don't attempt to create
// a tmpDir with a duplicate name
File tmpDir = new File(tmpFile + ".dir");
Assert.assertFalse(tmpDir.exists()); // never true if tmpfile does
it's job
Assert.assertTrue(tmpDir.mkdirs());
+// todo not every tmp directory needs this file
--- End diff --
Unfortunately this todo comment is in the master branch from where I
backported these changes in ClientBase.
---
[GitHub] zookeeper issue #459: ZOOKEEPER-2967: Add check to validate dataDir and data...
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/459 @afine I've removed the whitespace changes. Please have a look if it looks right now. ---
[GitHub] zookeeper pull request #462: ZOOKEEPER-2980 Backport ZOOKEEPER-2939 Deal wit...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/462#discussion_r168271141 --- Diff: build.xml --- @@ -220,7 +220,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle"> - + --- End diff -- Ok ---
[GitHub] zookeeper pull request #462: ZOOKEEPER-2980 Backport ZOOKEEPER-2939 Deal wit...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/462#discussion_r168179473 --- Diff: build.xml --- @@ -220,7 +220,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle"> - + --- End diff -- This version change comes from master/3.5. Do we really intend to upgrade to JUnit version 4.12 in this PR? ---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Send a SNAP if transactions can...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r167835407
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java ---
@@ -758,6 +760,11 @@ public boolean syncFollower(long peerLastZxid,
ZKDatabase db, Leader leader) {
currentZxid = maxCommittedLog;
needOpPacket = false;
needSnap = false;
+} else if (peerLastEpoch != lastProcessedEpoch &&
!db.isInCommittedLog(peerLastZxid)) {
--- End diff --
Could you please add a description to the comments above (to "Here are the
cases that we want to handle") what this else if case is doing?
---
[GitHub] zookeeper issue #461: missing list
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/461 Please add a JIRA to this pull request. ---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Send a SNAP if transactions can...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r167513290
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java ---
@@ -758,6 +760,11 @@ public boolean syncFollower(long peerLastZxid,
ZKDatabase db, Leader leader) {
currentZxid = maxCommittedLog;
needOpPacket = false;
needSnap = false;
+} else if (peerLastEpoch != lastProcessedEpoch &&
!db.isInCommittedLog(peerLastZxid)) {
+//Be sure we do a snap, because if the epochs are not the
same we don't know what
+// could have happened in between and it may take a TRUNC
+ UPDATES to get them in SYNC
+LOG.debug("Will send SNAP to peer sid: {} epochs are too
our of sync local 0x{} remote 0x{}",
--- End diff --
I think there is a typo here: "our of sync"
---
[GitHub] zookeeper pull request #459: ZOOKEEPER-2967: Add check to validate dataDir a...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/459 ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup This PR adds a check to protect ZK against configuring dataDir and dataLogDir opposingly. When FileTxnSnapLog is created, it checks if transaction log directory contains snapshot files or vice versa, snapshot directory contains transaction log files. If so, the check throws LogdirContentCheckException or SnapdirContentCheckException, respectively, which translates to DatadirException at ZK startup in QuorumPeerMain and ZooKeeperServerMain. If the two directories are the same, then no check is done. For testing, I've added 4 new unit tests which cover the following cases: transaction log and snapshot directories are different and they are used correctly (no Exception) transaction log and snapshot directories are the same (in this case no check is done) transaction log and snapshot directories are different and transaction log directory contains snapshot files (LogdirContentCheckException -> ZK quits) transaction log and snapshot directories are different and snapshot directory contains transaction log files (SnapdirContentCheckException -> ZK quits) You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2967_3.4 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/459.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 #459 commit 3cebb86cf5b2ab7561ee1a25e6eb7cc10bb7a5b6 Author: Mark Fenes <mfenes@...> Date: 2018-01-19T23:16:07Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup commit a0ef2ce168e65919a2613e8e7dcb0effaf3f838a Author: Mark Fenes <mfenes@...> Date: 2018-01-20T00:57:13Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup Replaced |= as this operator is not short-circuit commit 3d978556fc525c9f93db5c73a59d2cf52f847d6c Author: Mark Fenes <mfenes@...> Date: 2018-01-29T23:08:11Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup FilenameFilter, includes dot in log/snapshot filename check, improved/shortened unit test code, added @Before and @After to set up and delete temporary test directories. commit 4096a2500961e3015a8ec009ecfa088b6642a1e8 Author: Mark Fenes <mfenes@...> Date: 2018-02-01T23:21:45Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup Replaced "log" and "snapshot" strings with constants FileTxnLog.LOG_FILE_PREFIX and FileSnap.SNAPSHOT_FILE_PREFIX. Used Util.makeLogName when creating new transaction log file to make sure creation and verification follows the same file name pattern. Added reference to ZOOKEEPER-2967 to explain why this check is needed. Cleanup/refactoring of unit test FileTxnSnapLogTest. commit 5d8b5d0462700c70d317bc3de3989a03081cba36 Author: Mark Fenes <mfenes@...> Date: 2018-02-07T12:53:35Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup Fixed possible NP issues. commit 666a1b0a1bd33406f26c29df1407f75c94675d0f Author: Mark Fenes <mfenes@...> Date: 2018-02-08T10:31:46Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup Required changes to backport ZOOKEEPER-2967 from master to 3.4. ---
[GitHub] zookeeper pull request #458: ZOOKEEPER-2967: Add check to validate dataDir a...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/458 ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup This PR adds a check to protect ZK against configuring dataDir and dataLogDir opposingly. When FileTxnSnapLog is created, it checks if transaction log directory contains snapshot files or vice versa, snapshot directory contains transaction log files. If so, the check throws LogdirContentCheckException or SnapdirContentCheckException, respectively, which translates to DatadirException at ZK startup in QuorumPeerMain and ZooKeeperServerMain. If the two directories are the same, then no check is done. For testing, I've added 4 new unit tests which cover the following cases: transaction log and snapshot directories are different and they are used correctly (no Exception) transaction log and snapshot directories are the same (in this case no check is done) transaction log and snapshot directories are different and transaction log directory contains snapshot files (LogdirContentCheckException -> ZK quits) transaction log and snapshot directories are different and snapshot directory contains transaction log files (SnapdirContentCheckException -> ZK quits) You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2967_3.5 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/458.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 #458 commit 9d7c91e298ca7a291f502fdc792a2a27bd066d31 Author: Mark Fenes <mfenes@...> Date: 2018-01-19T23:16:07Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup commit 1fc1eb4fadf3ce2d83e3cd81d97332e9bf96f95c Author: Mark Fenes <mfenes@...> Date: 2018-01-20T00:57:13Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup Replaced |= as this operator is not short-circuit commit 9ea2a73fe0719070bbf9411752dc4b08df5f01c4 Author: Mark Fenes <mfenes@...> Date: 2018-01-29T23:08:11Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup FilenameFilter, includes dot in log/snapshot filename check, improved/shortened unit test code, added @Before and @After to set up and delete temporary test directories. commit 19f1b67449825037cc266f1ea5999a82d7baa352 Author: Mark Fenes <mfenes@...> Date: 2018-02-01T23:21:45Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup Replaced "log" and "snapshot" strings with constants FileTxnLog.LOG_FILE_PREFIX and FileSnap.SNAPSHOT_FILE_PREFIX. Used Util.makeLogName when creating new transaction log file to make sure creation and verification follows the same file name pattern. Added reference to ZOOKEEPER-2967 to explain why this check is needed. Cleanup/refactoring of unit test FileTxnSnapLogTest. commit 9b04f3b1e5b05eed80ba2c2df90770fb35bafbef Author: Mark Fenes <mfenes@...> Date: 2018-02-07T12:53:35Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup Fixed possible NP issues. commit 4468470a9483468a739a2cc7583e9d10ef7f233b Author: Mark Fenes <mfenes@...> Date: 2018-02-07T15:24:42Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup Required changes to backport ZOOKEEPER-2967 from master to 3.5. ---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r165525848
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java ---
@@ -136,13 +136,51 @@ public FileTxnSnapLog(File dataDir, File snapDir)
throws IOException {
throw new DatadirException("Cannot write to snap directory " +
this.snapDir);
}
+// check content of transaction log and snapshot dirs if they are
two different directories
--- End diff --
Added reference to ZOOKEEPER-2967 in the comments.
---
[GitHub] zookeeper issue #450: ZOOKEEPER-2967: Add check to validate dataDir and data...
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/450 @afine I've made the following changes: Replaced "log" and "snapshot" strings with constants FileTxnLog.LOG_FILE_PREFIX and FileSnap.SNAPSHOT_FILE_PREFIX. Used Util.makeLogName when creating new transaction log file to make sure creation and verification follows the same file name pattern. Added reference to ZOOKEEPER-2967 to explain why this check is needed. Cleaned up/refactored unit test FileTxnSnapLogTest to make it less verbose and consistent. ---
[GitHub] zookeeper issue #443: ZOOKEEPER-2955: Enable Clover code coverage report
Github user mfenes commented on the issue:
https://github.com/apache/zookeeper/pull/443
@phunt @afine I've made the following changes: moved Clover (db, lib,
reports) to ${build.dir}/clover. Included test sources with `` in
clover-setup.
Changed naming to follow other tools and updated/added ant target
descriptions.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164606812
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -159,4 +159,222 @@ public void onTxnLoaded(TxnHeader hdr, Record rec) {
}
}
}
+
+@Test
+public void testDirCheckWithCorrectFiles() throws IOException {
+File tmpDir = ClientBase.createEmptyTestDir();
+File logDir = new File(tmpDir, "logdir");
+File snapDir = new File(tmpDir, "snapdir");
+File logVersionDir = new File(logDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+File snapVersionDir = new File(snapDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+
+if (!logVersionDir.exists()) {
+logVersionDir.mkdirs();
+}
+if (!snapVersionDir.exists()) {
+snapVersionDir.mkdirs();
+}
+
+Assert.assertTrue(logVersionDir.exists());
+Assert.assertTrue(snapVersionDir.exists());
+
+// transaction log files in log dir - correct
+File logFile1 = new File(logVersionDir.getPath() +File.separator +
Util.makeLogName(1L));
+logFile1.createNewFile();
+File logFile2 = new File(logVersionDir.getPath() +File.separator +
Util.makeLogName(2L));
+logFile2.createNewFile();
+
+// snapshot files in snap dir - correct
+File snapFile1 = new File(snapVersionDir.getPath() +File.separator
+ Util.makeSnapshotName(1L));
+snapFile1.createNewFile();
+File snapFile2 = new File(snapVersionDir.getPath() +File.separator
+ Util.makeSnapshotName(2L));
+snapFile2.createNewFile();
+
+Assert.assertTrue(logFile1.exists());
+Assert.assertTrue(logFile2.exists());
+Assert.assertTrue(snapFile1.exists());
+Assert.assertTrue(snapFile2.exists());
+
+String priorAutocreateDirValue =
System.getProperty(FileTxnSnapLog.ZOOKEEPER_DATADIR_AUTOCREATE);
+System.setProperty(FileTxnSnapLog.ZOOKEEPER_DATADIR_AUTOCREATE,
"false");
+FileTxnSnapLog fileTxnSnapLog;
+try {
+fileTxnSnapLog = new FileTxnSnapLog(logDir, snapDir);
+} catch (FileTxnSnapLog.LogdirContentCheckException e) {
+Assert.fail("Should not throw LogdirContentCheckException.");
+} catch (FileTxnSnapLog.SnapdirContentCheckException e) {
+Assert.fail("Should not throw SnapdirContentCheckException.");
+} finally {
+if (priorAutocreateDirValue == null) {
+
System.clearProperty(FileTxnSnapLog.ZOOKEEPER_DATADIR_AUTOCREATE);
+} else {
+
System.setProperty(FileTxnSnapLog.ZOOKEEPER_DATADIR_AUTOCREATE,
priorAutocreateDirValue);
+}
--- End diff --
I switched off autocreate to make sure the test is reading the test input
data and not something else by mistake.
Added clean-up of temp directories.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605998
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -159,4 +159,222 @@ public void onTxnLoaded(TxnHeader hdr, Record rec) {
}
}
}
+
+@Test
+public void testDirCheckWithCorrectFiles() throws IOException {
+File tmpDir = ClientBase.createEmptyTestDir();
+File logDir = new File(tmpDir, "logdir");
+File snapDir = new File(tmpDir, "snapdir");
+File logVersionDir = new File(logDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+File snapVersionDir = new File(snapDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+
+if (!logVersionDir.exists()) {
+logVersionDir.mkdirs();
+}
+if (!snapVersionDir.exists()) {
+snapVersionDir.mkdirs();
+}
+
+Assert.assertTrue(logVersionDir.exists());
+Assert.assertTrue(snapVersionDir.exists());
+
+// transaction log files in log dir - correct
+File logFile1 = new File(logVersionDir.getPath() +File.separator +
Util.makeLogName(1L));
+logFile1.createNewFile();
+File logFile2 = new File(logVersionDir.getPath() +File.separator +
Util.makeLogName(2L));
+logFile2.createNewFile();
+
+// snapshot files in snap dir - correct
+File snapFile1 = new File(snapVersionDir.getPath() +File.separator
+ Util.makeSnapshotName(1L));
+snapFile1.createNewFile();
+File snapFile2 = new File(snapVersionDir.getPath() +File.separator
+ Util.makeSnapshotName(2L));
+snapFile2.createNewFile();
+
+Assert.assertTrue(logFile1.exists());
+Assert.assertTrue(logFile2.exists());
+Assert.assertTrue(snapFile1.exists());
+Assert.assertTrue(snapFile2.exists());
+
+String priorAutocreateDirValue =
System.getProperty(FileTxnSnapLog.ZOOKEEPER_DATADIR_AUTOCREATE);
+System.setProperty(FileTxnSnapLog.ZOOKEEPER_DATADIR_AUTOCREATE,
"false");
+FileTxnSnapLog fileTxnSnapLog;
+try {
+fileTxnSnapLog = new FileTxnSnapLog(logDir, snapDir);
+} catch (FileTxnSnapLog.LogdirContentCheckException e) {
+Assert.fail("Should not throw LogdirContentCheckException.");
+} catch (FileTxnSnapLog.SnapdirContentCheckException e) {
--- End diff --
Done.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605964
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -159,4 +159,222 @@ public void onTxnLoaded(TxnHeader hdr, Record rec) {
}
}
}
+
+@Test
+public void testDirCheckWithCorrectFiles() throws IOException {
+File tmpDir = ClientBase.createEmptyTestDir();
+File logDir = new File(tmpDir, "logdir");
+File snapDir = new File(tmpDir, "snapdir");
+File logVersionDir = new File(logDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+File snapVersionDir = new File(snapDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+
+if (!logVersionDir.exists()) {
+logVersionDir.mkdirs();
+}
+if (!snapVersionDir.exists()) {
+snapVersionDir.mkdirs();
+}
+
+Assert.assertTrue(logVersionDir.exists());
+Assert.assertTrue(snapVersionDir.exists());
+
+// transaction log files in log dir - correct
+File logFile1 = new File(logVersionDir.getPath() +File.separator +
Util.makeLogName(1L));
+logFile1.createNewFile();
+File logFile2 = new File(logVersionDir.getPath() +File.separator +
Util.makeLogName(2L));
+logFile2.createNewFile();
+
+// snapshot files in snap dir - correct
+File snapFile1 = new File(snapVersionDir.getPath() +File.separator
+ Util.makeSnapshotName(1L));
+snapFile1.createNewFile();
+File snapFile2 = new File(snapVersionDir.getPath() +File.separator
+ Util.makeSnapshotName(2L));
+snapFile2.createNewFile();
+
+Assert.assertTrue(logFile1.exists());
+Assert.assertTrue(logFile2.exists());
+Assert.assertTrue(snapFile1.exists());
+Assert.assertTrue(snapFile2.exists());
--- End diff --
Ok, removed.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605860
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -159,4 +159,222 @@ public void onTxnLoaded(TxnHeader hdr, Record rec) {
}
}
}
+
+@Test
+public void testDirCheckWithCorrectFiles() throws IOException {
+File tmpDir = ClientBase.createEmptyTestDir();
+File logDir = new File(tmpDir, "logdir");
+File snapDir = new File(tmpDir, "snapdir");
+File logVersionDir = new File(logDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+File snapVersionDir = new File(snapDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+
+if (!logVersionDir.exists()) {
+logVersionDir.mkdirs();
+}
+if (!snapVersionDir.exists()) {
--- End diff --
I've removed this, too.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605834
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -159,4 +159,222 @@ public void onTxnLoaded(TxnHeader hdr, Record rec) {
}
}
}
+
+@Test
+public void testDirCheckWithCorrectFiles() throws IOException {
+File tmpDir = ClientBase.createEmptyTestDir();
+File logDir = new File(tmpDir, "logdir");
+File snapDir = new File(tmpDir, "snapdir");
+File logVersionDir = new File(logDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+File snapVersionDir = new File(snapDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+
+if (!logVersionDir.exists()) {
--- End diff --
I've removed this.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605909
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -159,4 +159,222 @@ public void onTxnLoaded(TxnHeader hdr, Record rec) {
}
}
}
+
+@Test
+public void testDirCheckWithCorrectFiles() throws IOException {
+File tmpDir = ClientBase.createEmptyTestDir();
+File logDir = new File(tmpDir, "logdir");
+File snapDir = new File(tmpDir, "snapdir");
+File logVersionDir = new File(logDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+File snapVersionDir = new File(snapDir, FileTxnSnapLog.version +
FileTxnSnapLog.VERSION);
+
+if (!logVersionDir.exists()) {
+logVersionDir.mkdirs();
+}
+if (!snapVersionDir.exists()) {
+snapVersionDir.mkdirs();
+}
+
+Assert.assertTrue(logVersionDir.exists());
+Assert.assertTrue(snapVersionDir.exists());
--- End diff --
Ok, removed.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605723
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/Util.java
---
@@ -294,5 +297,25 @@ public int compare(File o1, File o2) {
Collections.sort(filelist, new DataDirFileComparator(prefix,
ascending));
return filelist;
}
+
+/**
+ * Returns true if file is a log file.
+ *
+ * @param file
+ * @return
+ */
+public static boolean isLogFile(File file) {
+return file.getName().startsWith(LOG_FILE_PREFIX);
+}
+
+/**
+ * Returns true if file is a snapshot file.
+ *
+ * @param file
+ * @return
+ */
+public static boolean isSnapshotFile(File file) {
+return file.getName().startsWith(SNAP_FILE_PREFIX);
--- End diff --
Done.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605619
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/Util.java
---
@@ -294,5 +297,25 @@ public int compare(File o1, File o2) {
Collections.sort(filelist, new DataDirFileComparator(prefix,
ascending));
return filelist;
}
+
+/**
+ * Returns true if file is a log file.
+ *
+ * @param file
+ * @return
+ */
+public static boolean isLogFile(File file) {
+return file.getName().startsWith(LOG_FILE_PREFIX);
--- End diff --
This is done.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605521
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java ---
@@ -136,13 +136,51 @@ public FileTxnSnapLog(File dataDir, File snapDir)
throws IOException {
throw new DatadirException("Cannot write to snap directory " +
this.snapDir);
}
+// check content of transaction log and snapshot dirs if they are
two different directories
+if(!this.dataDir.getPath().equals(this.snapDir.getPath())){
+checkLogDir();
+checkSnapDir();
+}
+
txnLog = new FileTxnLog(this.dataDir);
snapLog = new FileSnap(this.snapDir);
autoCreateDB =
Boolean.parseBoolean(System.getProperty(ZOOKEEPER_DB_AUTOCREATE,
ZOOKEEPER_DB_AUTOCREATE_DEFAULT));
}
+private void checkLogDir() throws LogdirContentCheckException {
+File[] files = this.dataDir.listFiles();
--- End diff --
Changed implementation to use FilenameFilter.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r164605358
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -159,4 +159,222 @@ public void onTxnLoaded(TxnHeader hdr, Record rec) {
}
}
}
+
+@Test
--- End diff --
@afine I've improved/shortened the test code
---
[GitHub] zookeeper issue #451: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/451 Looking at the static initialization block in InetAddressCachePolicy more deeply, the default TTL is 30 seconds if there is no SecurityManager installed. So caching a positive lookup forever in the Java-level cache is the default only if there is a SecurityManager installed and the TTL is not overridden by "networkaddress.cache.ttl" to a different value. Default caching policy for a negative lookup is 0 (never cache). Now the only question is whether 30 seconds default caching is ok or too much for ZK. ---
[GitHub] zookeeper issue #451: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/451 Re-resolving at StaticHostProvider level may not be sufficient as InetAddress.getAllByName(String host) itself uses a Java-level cache inside InetAddress and turns to name service (e.g. DNS) only if the host could not be found in the Java-level cache. Unfortunately, when Java resolves a new host using the name service, it puts the host and its addresses in the cache with TTL cache FOREVER. This means, once a host gets resolved by Java, it will never again turn to the name service to re-resolve it. If a host's addresses get updated in DNS, the address cache in Java will still contain the old entry forever. So re-resolving at StaticHostProvider won't help in this case, as InetAddress.getAllByName(String host) will still return the old address(es) I think. Check the getCachedAddresses method inside InetAddress, the get() method of static final class Cache inside InetAddress and sun.net.InetAddressCachePolicy.get() which returns cachePolicy with default value -1 (FOREVER) if it is not overridden by Security properties "networkaddress.cache.ttl" and "networkaddress.cache.negative.ttl". ---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/443#discussion_r163684223 --- Diff: build.xml --- @@ -23,6 +23,48 @@ xmlns:artifact="antlib:org.apache.maven.artifact.ant" xmlns:maven="antlib:org.apache.maven.artifact.ant" xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- @afine Because I need `` in `` and the dependency versions at their original location were declared later than clover.jar, so clover.version was undefined at this point. I had 3 choices: 1. move the clover property set behind the dependency version declarations just before the macro definitions, separating it from the other property set declarations, 1. move all the dependency version declarations before the clover property set, 1. or just declare the clover.version separately from the other dependency versions in the clover property set block. I've chosen option 2 as I thought that was the best out of the three. Please let me know if there is a better way to solve this. ---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/443#discussion_r163678316 --- Diff: build.xml --- @@ -1406,50 +1410,53 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- @phunt Actually, if you prefer, I could rename both the ivy configuration and the ant target for clover to "test-coverage-clover-java" as this does not exclude the possibility of including it into a test-coverage-java target later. ---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/443#discussion_r163675784
--- Diff: build.xml ---
@@ -124,6 +160,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
+
--- End diff --
Ok. So would changing the location of Clover to ${build.dir}/clover/lib
solve this problem?
* Are you ok with the following directory structure:
build/clover/db for Clover database,
build/clover/jar for Clover jar,
build/clover/reports for Clover reports, and
clover.home=${build.dir}/clover
* Should I change property name "ivy.coverage.lib" to "ivy.clover.lib"?
---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/443#discussion_r163670562 --- Diff: build.xml --- @@ -1406,50 +1410,53 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- Because the "clover" target does not generate the HTML and XML reports. It's used to initialise and setup Clover and it's called as a dependency of "compile" to instrument source code. 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. The reason behind the naming was that I'm also planning to add C code coverage tests, and following the already existing ant target names "test-core-java" and "test-core-cppunit" as naming patterns, there could be targets "test-coverage-java" and "test-coverage-cppunit" to generate coverage reports for Java and C, respectively. Their parent target could be "test-coverage", which would run "test-coverage-java" and "test-coverage-cppunit" to prepare a complete coverage report for both Java and C. Then running a full coverage report for ZK would be as simple as running "test-coverage". Please let me know what you think. ---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r163290319
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/Util.java
---
@@ -294,5 +297,25 @@ public int compare(File o1, File o2) {
Collections.sort(filelist, new DataDirFileComparator(prefix,
ascending));
return filelist;
}
+
+/**
+ * Returns true if file is a log file.
+ *
+ * @param file
+ * @return
+ */
+public static boolean isLogFile(File file) {
+return file.getName().startsWith(LOG_FILE_PREFIX);
+}
+
+/**
+ * Returns true if file is a snapshot file.
+ *
+ * @param file
+ * @return
+ */
+public static boolean isSnapshotFile(File file) {
+return file.getName().startsWith(SNAP_FILE_PREFIX);
--- End diff --
Ok, I'll include the dot in the check.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r163290119
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/Util.java
---
@@ -294,5 +297,25 @@ public int compare(File o1, File o2) {
Collections.sort(filelist, new DataDirFileComparator(prefix,
ascending));
return filelist;
}
+
+/**
+ * Returns true if file is a log file.
+ *
+ * @param file
+ * @return
+ */
+public static boolean isLogFile(File file) {
+return file.getName().startsWith(LOG_FILE_PREFIX);
--- End diff --
I agree that it might be safer to check with the dot. I'll add this change.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r163289284
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/Util.java
---
@@ -83,7 +86,7 @@ public static URI makeFileLoggerURL(File dataDir, File
dataLogDir,String convPol
* @return file name
*/
public static String makeLogName(long zxid) {
-return "log." + Long.toHexString(zxid);
+return LOG_FILE_PREFIX + "." + Long.toHexString(zxid);
--- End diff --
I deliberately excluded the dot from LOG_FILE_PREFIX and SNAP_FILE_PREFIX
after I checked that in FileTxnLog and FileSnap classes the prefix argument
(like "log", "snapshot") in method calls is passed without the dot. I wanted to
avoid confusion.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r163282674
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java ---
@@ -136,13 +136,51 @@ public FileTxnSnapLog(File dataDir, File snapDir)
throws IOException {
throw new DatadirException("Cannot write to snap directory " +
this.snapDir);
}
+// check content of transaction log and snapshot dirs if they are
two different directories
+if(!this.dataDir.getPath().equals(this.snapDir.getPath())){
+checkLogDir();
+checkSnapDir();
+}
+
txnLog = new FileTxnLog(this.dataDir);
snapLog = new FileSnap(this.snapDir);
autoCreateDB =
Boolean.parseBoolean(System.getProperty(ZOOKEEPER_DB_AUTOCREATE,
ZOOKEEPER_DB_AUTOCREATE_DEFAULT));
}
+private void checkLogDir() throws LogdirContentCheckException {
+File[] files = this.dataDir.listFiles();
+if(files != null) {
+boolean hasSnapshotFiles = false;
+for (File file : files) {
+if(Util.isSnapshotFile(file)){
+hasSnapshotFiles = true;
+break;
+}
+}
+if (hasSnapshotFiles) {
+throw new LogdirContentCheckException("Log directory has
snapshot files. Check if dataLogDir and dataDir configuration is correct.");
+}
+}
+}
+
+private void checkSnapDir() throws SnapdirContentCheckException {
+File[] files = this.snapDir.listFiles();
--- End diff --
Please see my comments above.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r163282090
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java ---
@@ -136,13 +136,51 @@ public FileTxnSnapLog(File dataDir, File snapDir)
throws IOException {
throw new DatadirException("Cannot write to snap directory " +
this.snapDir);
}
+// check content of transaction log and snapshot dirs if they are
two different directories
+if(!this.dataDir.getPath().equals(this.snapDir.getPath())){
+checkLogDir();
+checkSnapDir();
+}
+
txnLog = new FileTxnLog(this.dataDir);
snapLog = new FileSnap(this.snapDir);
autoCreateDB =
Boolean.parseBoolean(System.getProperty(ZOOKEEPER_DB_AUTOCREATE,
ZOOKEEPER_DB_AUTOCREATE_DEFAULT));
}
+private void checkLogDir() throws LogdirContentCheckException {
+File[] files = this.dataDir.listFiles();
--- End diff --
If I used FilenameFilter, then Util.isSnapshotFile() / Util.isLogFile()
check would be run for all the files in the directory and
listFiles(FilenameFilter filter) would return all the files satisfying the
filter condition, however I need only the first occurrence which satisfies the
condition, not all of them. The current logic quits from the for loop
immediately when it finds a file violating the configuration and throws an
exception, while your proposal would iterate over all the files in the
directory and would call Util.isSnapshotFile() / Util.isLogFile() for each of
the files inside FilenameFilter to prepare the filtered File[]. So using
FilenameFilter would be a bit slower, but yes, it might need less lines in
code, also at the price of obscuring the purpose of the code (i.e.
hasSnapshotFiles / hasLogFiles boolean variables tell what the problem exactly
is, while if (snapshotFiles.length > 0) { throw new Exception(...) } would
not). However, if we prefer using Java library
classes over standard coding patterns even in cases when it does not fit the
purpose entirely, then FilenameFilter can be the winner.
---
[GitHub] zookeeper issue #443: ZOOKEEPER-2955: Enable Clover code coverage report
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/443 @phunt I've fixed "ant compile-test". Now clover dependencies are pulled into build only when run.clover=true. Thanks for noticing this. ---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/443#discussion_r163218498
--- Diff: build.xml ---
@@ -132,21 +169,16 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
-
-
-
-
-
-
-
-
-
-
-
+
+
--- End diff --
The main reason why I set clover.home to ${test.java.build.dir} is because
originally in master, 3.5 and 3.4 Clover db is set to
${test.java.build.dir}/clover/db and Clover reports are set to
${test.java.build.dir}/clover/reports. This indicates that clover.home should
be ${test.java.build.dir}. Since Clover was in use before this change, I
thought it's better to keep the original settings as other tools (e.g. a
Jenkins job) might rely on reports being generated under this location.
@anmolnar also recommended to use this setting in his comment. Setting
clover.home to ${test.java.build.dir} does not put everything related to Clover
directly under build/test, as Clover db goes into build/test/clover/db, Clover
reports go under build/test/clover/reports and clover jar is placed in
/build/test/lib. Of course, I can change clover.home to point to any other
location, e.g. to build/coverage, but one reason against it might be that
Clover code coverage is more related to test than to bui
ld.
---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user mfenes commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/443#discussion_r163198966 --- Diff: build.xml --- @@ -124,6 +160,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- I did see the naming inconsistency, but on the other hand code coverage measurement belongs to testing, and everything related to testing is placed under build/test. Not all the tools have their own subdirectory under the build directory (like javacc, releaseaudit, owasp), e.g. JUnit does not have. If JUnit does not have its own subdirectory under build/, then why should OpenClover have. If the reason to put OpenClover under an own build/coverage directory instead of putting it into build/test/lib is packaging, i.e. not delivering the clover jar in releases, then basically the answer is that a ZK compiled with Clover should not be released. I also verified that ant tar does not include build/test/lib jars. If ant targets tar, jar, compile, test-core-java etc. are run without the -Drun.clover=true parameter, then the clover jar is not even retrieved by ivy, so it won't be included, everything will run as before. Another reason is, that originally in current code everything related to Clover (db, reports) is configured to be under build/test/clover. If Clover db and reports are under build/test/clover, then why should the clover jar be put in build/coverage/lib and not in build/test/lib? All-in-all, these were the reasons behind my decision to put Clover under build/test/lib instead of build/coverage/lib. However, I agree that this is a naming inconsistency and I can see the reason that everything which has its own ivy configuration should have its own directory under build, so I can change the location of Clover if you prefer having Clover under build/coverage/lib. ---
[GitHub] zookeeper issue #443: ZOOKEEPER-2955: Enable Clover code coverage report
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/443 @phunt I've added ivy.xml configuration for Clover: test-coverage-java and created an ant target with the same name, so generating a coverage report for Java became as simple as running ant test-coverage-java. ---
[GitHub] zookeeper issue #443: ZOOKEEPER-2955: Enable Clover code coverage report
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/443 @anmolnar Clover home is now set to test.java.build.dir. No need to configure CLOVER_HOME environment variable. ---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/450 ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir paramete⦠ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup This PR adds a check to protect ZK against configuring dataDir and dataLogDir opposingly. When FileTxnSnapLog is created, it checks if transaction log directory contains snapshot files or vice versa, snapshot directory contains transaction log files. If so, the check throws LogdirContentCheckException or SnapdirContentCheckException, respectively, which translates to DatadirException at ZK startup in QuorumPeerMain and ZooKeeperServerMain. If the two directories are the same, then no check is done. For testing, I've added 4 new unit tests which cover the following cases: 1. transaction log and snapshot directories are different and they are used correctly (no Exception) 2. transaction log and snapshot directories are the same (in this case no check is done) 3. transaction log and snapshot directories are different and transaction log directory contains snapshot files (LogdirContentCheckException -> ZK quits) 4. transaction log and snapshot directories are different and snapshot directory contains transaction log files (SnapdirContentCheckException -> ZK quits) You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2967 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/450.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 #450 commit 81c026fe8498107f42e9d3599a515c8817f8bf02 Author: Mark Fenes <mfenes@...> Date: 2018-01-19T23:16:07Z ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at startup ---
[GitHub] zookeeper issue #443: ZOOKEEPER-2955: Enable Clover code coverage report
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/443 @anmolnar Yes, OpenClover's Intellectual property protection page http://openclover.org/doc/manual/4.2.0/developer-guide--intellectual-property-protection.html says that - Clover Core is licensed under Apache 2.0 License - Most of Clover plugins are licensed under Apache 2.0 License - Bamboo Clover Plugin is proprietary software, see Bamboo Clover Plugin Developer Guide - Clover documentation on confluence.atlassian.com is under Creative Commons License, see Contributing to the Clover Documentation ---
[GitHub] zookeeper issue #443: ZOOKEEPER-2955: Enable Clover code coverage report
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/443 @phunt According to OpenClover 4.2.0 Release Notes at http://openclover.org/doc/openclover-4.2.0-release-notes.html _"No license key required OpenClover requires no license key, in some places you may see a "Clover free edition" message. You no longer need any external license key to run it. In case you still pass the license key issued by Atlassian in your builds (e.g. via -Dclover.license.path JVM property), it will be ignored and the built-in one will be used."_ Regarding being able to run OpenClover on Apache Jenkins: I think we can configure a Jenkins job to generate code coverage reports since OpenClover does not need any installation, it's just a clover-4.2.1.jar file in /build/test/lib. However we would need a storage space to store the generated coverage reports as otherwise it would be deleted when the /build directory is cleaned. Also I would mention that OpenClover needs ZK to be compiled using the -Drun.clover=true option in order to use source code instrumentation and ZK compiled with source code instrumentation should not be used for production, only just for testing and generating coverage reports. ---
[GitHub] zookeeper pull request #445: ZOOKEEPER-2955: Enable Clover code coverage rep...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/445 ZOOKEEPER-2955: Enable Clover code coverage report ZOOKEEPER-2955: Enable Clover code coverage report This PR configures OpenClover to generate Java code coverage reports. To enable OpenClover run the following ant targets with -Drun.clover=true: ant -Drun.clover=true jar ant -Drun.clover=true test and then to generate the code coverage reports run: ant -Drun.clover=true generate-clover-reports The reports will be generated under the build/test/clover/reports directory in HTML and XML formats. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2955_3.4 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/445.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 #445 commit 7bda91c4dbaaabdb3df29730bc7af3855b065453 Author: Mark Fenes <mfenes@...> Date: 2018-01-05T13:21:19Z ZOOKEEPER-2955: Enable Clover code coverage report ---
[GitHub] zookeeper pull request #444: ZOOKEEPER-2955: Enable Clover code coverage rep...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/444 ZOOKEEPER-2955: Enable Clover code coverage report ZOOKEEPER-2955: Enable Clover code coverage report This PR configures OpenClover to generate Java code coverage reports. To enable OpenClover run the following ant targets with -Drun.clover=true: ant -Drun.clover=true jar ant -Drun.clover=true test and then to generate the code coverage reports run: ant -Drun.clover=true generate-clover-reports The reports will be generated under the build/test/clover/reports directory in HTML and XML formats. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2955_3.5 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/444.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 #444 commit e12114ad5a6fa47b201852a82175a143439eb5db Author: Mark Fenes <mfenes@...> Date: 2018-01-05T13:21:19Z ZOOKEEPER-2955: Enable Clover code coverage report ---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/443 ZOOKEEPER-2955: Enable Clover code coverage report ZOOKEEPER-2955: Enable Clover code coverage report This PR configures OpenClover to generate Java code coverage reports. To enable OpenClover run the following ant targets with -Drun.clover=true: ant -Drun.clover=true jar ant -Drun.clover=true test and then to generate the code coverage reports run: ant -Drun.clover=true generate-clover-reports The reports will be generated under the build/test/clover/reports directory in HTML and XML formats. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2955 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/443.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 #443 commit f59dcf8ade02ffbe693b56b480d60aff821900de Author: Mark Fenes <mfenes@...> Date: 2018-01-05T13:21:19Z ZOOKEEPER-2955: Enable Clover code coverage report ---
[GitHub] zookeeper issue #427: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/427 @fr0stbyte could you please add a description to your PR so that everybody could understand the purpose of this change? ---
[GitHub] zookeeper issue #423: ZOOKEEPER-2949: using hostname and port to create SSLE...
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/423 Could you please provide a description of your change ---
[GitHub] zookeeper pull request #404: ZOOKEEPER-2690: Update documentation source for...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/404 ---
[GitHub] zookeeper pull request #405: ZOOKEEPER-2690: Update documentation source for...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/405 ---
[GitHub] zookeeper pull request #406: ZOOKEEPER-2690: Update documentation source for...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/406 ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Added the documentation changes from PR #111 to the source (zookeeperAdmin.xml) and generated the new version of the html and pdf documents. Note: I have not updated the 2nd paragraph from ZOOKEEPER-2574 as change "ZOOKEEPER-2349: Update documentation for snapCount" has a more recent version of that part of the text. (ZOOKEEPER-2349 was committed on Sep 11, 2017 while ZOOKEEPER-2574 was committed on Jan 23, 2017.) You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2690_master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/406.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 #406 commit b152f04d7fdc97e65fef1a6f62fe72758778b3bf Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-10-19T20:42:29Z ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Change-Id: I99f450beda806598bb004cb8d3b0ec11e82ae588 commit ba933658c440b1e33f42389a8d3ac7e496640ff9 Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-10-25T13:48:49Z ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Generated documents Change-Id: I33ff0ef2410fead9755e3623b4f349f57b9f8121 ---
[GitHub] zookeeper pull request #405: ZOOKEEPER-2690: Update documentation source for...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/405 ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Added the documentation changes from PR https://github.com/apache/zookeeper/pull/111/ to the source (zookeeperAdmin.xml) and generated the new version of the html and pdf documents. Note: I have not updated the 2nd paragraph from ZOOKEEPER-2574 as change "ZOOKEEPER-2349: Update documentation for snapCount" has a more recent version of that part of the text. (ZOOKEEPER-2349 was committed on Sep 11, 2017 while ZOOKEEPER-2574 was committed on Jan 23, 2017.) You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2690_3.4 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/405.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 #405 commit ada588bf16ea99d6fa39f6f7d61d883c437b2cc7 Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-10-18T23:54:34Z ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Change-Id: Id24b00dae078c1e9fee63add69a8ea700c157175 commit a7678f5671d64f6c68fc4d6127ddd21a9bb72ec8 Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-10-25T13:38:10Z ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Generated documents Change-Id: Icb12ab528f9f6fb062c5cfd4f57a8129504e69ee ---
[GitHub] zookeeper pull request #404: ZOOKEEPER-2690: Update documentation source for...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/404 ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Added the documentation changes from PR https://github.com/apache/zookeeper/pull/111/ to the source (zookeeperAdmin.xml) and generated the new version of the html and pdf documents. Note: I have not updated the 2nd paragraph from ZOOKEEPER-2574 as change "ZOOKEEPER-2349: Update documentation for snapCount" has a more recent version of that part of the text. (ZOOKEEPER-2349 was committed on Sep 11, 2017 while ZOOKEEPER-2574 was committed on Jan 23, 2017.) You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2690 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/404.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 #404 commit 0cb321966a0e53d29809ad91f1f9d9f354f99db2 Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-10-18T21:47:44Z ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Change-Id: I1bab70d91913d36f83efef755729640b7459e2e5 commit 87c53567f5f967fa97daef3cbc4f369af36a4aa3 Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-10-25T13:29:55Z ZOOKEEPER-2690: Update documentation source for ZOOKEEPER-2574 Generated documents Change-Id: I712f3f5f6ed3dbb41c9e69cac260f3c3047a651d ---
[GitHub] zookeeper pull request #375: ZOOKEEPER-1363: Categorise unit tests by 'test-...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/375 ---
[GitHub] zookeeper pull request #375: ZOOKEEPER-1363: Categorise unit tests by 'test-...
GitHub user mfenes reopened a pull request:
https://github.com/apache/zookeeper/pull/375
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
Added new capability of defining test categories in test category
configuration files. Test category configuration files can contain list of test
file names and/or patterns. It is also possible to exclude tests from a test
category. Test categories are added to ${test.src.dir}/category directory.
Current test categories are defined so that they remained API compatible with
the previous build.xml in regards to how âant testâ should be run (options
are the same and they behave the same way).
Func and Perf categories are added to keep the previous FuncTest and
PerfTest categories which were coded into the test file names.
Slow test category is added and contains tests which require more than 30
seconds to complete on a local developer machine.
Command âant test -Dtest.quick=yesâ runs tests minus tests in the Slow
category.
Command âant test -Dtest.category=â runs tests from the
specified category.
It is possible to combine these two options to run a test category minus
the Slow tests.
Test category file name must match the name of the test category (e.g.
category/Commit).
List of excluded tests/patterns should be added to a .exclude file having
the same name as the category (e.g. category/Commit.exclude).
Added README.txt which describes how to run unit tests in detail.
Benefits:
- test categories do not need to be included in the test file names (a
test can be part of more than just one category).
- Slow test category is more flexible to exclude slow tests than the
hardcoded exclusion of â**/*HammerTest.javaâ in build.xml.
- new test categorization can also exclude test file names/patterns from a
category.
- test category can be added for a patch.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-1363
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/375.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 #375
commit cd51fe57cd839f49bf7babdae4ebded3c92f4b26
Author: Mark Fenes <mfe...@cloudera.com>
Date: 2017-09-08T15:00:29Z
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
commit 30414528e8550fa4835beb7f8d901f8cad773d6e
Author: Mark Fenes <mfe...@cloudera.com>
Date: 2017-10-24T14:37:29Z
Trigger notification
Change-Id: I38ad5771552ff2bad16271dfb05fa00a683c3228
---
[GitHub] zookeeper pull request #375: ZOOKEEPER-1363: Categorise unit tests by 'test-...
GitHub user mfenes reopened a pull request:
https://github.com/apache/zookeeper/pull/375
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
Added new capability of defining test categories in test category
configuration files. Test category configuration files can contain list of test
file names and/or patterns. It is also possible to exclude tests from a test
category. Test categories are added to ${test.src.dir}/category directory.
Current test categories are defined so that they remained API compatible with
the previous build.xml in regards to how âant testâ should be run (options
are the same and they behave the same way).
Func and Perf categories are added to keep the previous FuncTest and
PerfTest categories which were coded into the test file names.
Slow test category is added and contains tests which require more than 30
seconds to complete on a local developer machine.
Command âant test -Dtest.quick=yesâ runs tests minus tests in the Slow
category.
Command âant test -Dtest.category=â runs tests from the
specified category.
It is possible to combine these two options to run a test category minus
the Slow tests.
Test category file name must match the name of the test category (e.g.
category/Commit).
List of excluded tests/patterns should be added to a .exclude file having
the same name as the category (e.g. category/Commit.exclude).
Added README.txt which describes how to run unit tests in detail.
Benefits:
- test categories do not need to be included in the test file names (a
test can be part of more than just one category).
- Slow test category is more flexible to exclude slow tests than the
hardcoded exclusion of â**/*HammerTest.javaâ in build.xml.
- new test categorization can also exclude test file names/patterns from a
category.
- test category can be added for a patch.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-1363
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/375.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 #375
commit cd51fe57cd839f49bf7babdae4ebded3c92f4b26
Author: Mark Fenes <mfe...@cloudera.com>
Date: 2017-09-08T15:00:29Z
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
---
[GitHub] zookeeper pull request #375: ZOOKEEPER-1363: Categorise unit tests by 'test-...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/375 ---
[GitHub] zookeeper issue #371: ZOOKEEPER-2814: Ignore space after comma in connection...
Github user mfenes commented on the issue:
https://github.com/apache/zookeeper/pull/371
Regex implementation would be shorter to ignore the spaces and empty
strings as
```
connectString.replaceAll("\\s", "").replaceAll(",+", ",").split(",")
```
does the same thing as the new StringUtils.split(String value, String
separator) implementation in this PR.
---
[GitHub] zookeeper pull request #390: ZOOKEEPER-2908: quorum.auth.MiniKdcTest.testKer...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/390 ---
[GitHub] zookeeper pull request #391: ZOOKEEPER-2727: WARN and stacktrace for normall...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/391 ZOOKEEPER-2727: WARN and stacktrace for normally closed socket ZOOKEEPER-2727: WARN and stacktrace for normally closed socket Log level reduced to info for EndOfStreamException as asked in the Jira. Logging stack trace for EndOfStreamException has been already moved to debug level in an earlier commit: https://github.com/apache/zookeeper/commit/34665cd5bdbcb6aaeecb6b204028ef1ffab9f2d8 You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2727 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/391.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 #391 commit ab3b6a0de3442fbf782984c0b35de52c76b0974d Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-10-04T21:13:26Z ZOOKEEPER-2727: WARN and stacktrace for normally closed socket ---
[GitHub] zookeeper pull request #355: ZOOKEEPER-2809: Unnecessary stack-trace in serv...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/355 ---
[GitHub] zookeeper issue #375: ZOOKEEPER-1363: Categorise unit tests by 'test-commit'...
Github user mfenes commented on the issue: https://github.com/apache/zookeeper/pull/375 The reason why I've created a test category for "slow" and not for "fast" is because this way one can specify a test category by ant test -Dtest.category=... and can exclude the "slow" tests from any category by using the -Dtest.quick=yes option. The special logic for "slow" is also needed for backwards compatibility with the current ant test options. ---
[GitHub] zookeeper pull request #390: ZOOKEEPER-2908: quorum.auth.MiniKdcTest.testKer...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/390 ZOOKEEPER-2908: quorum.auth.MiniKdcTest.testKerberosLogin failing wit⦠ZOOKEEPER-2908: quorum.auth.MiniKdcTest.testKerberosLogin failing with NPE on Java 9 Cause: The NPE exception in the MiniKdcTest.testKerberosLogin() unit test is caused by a duplicate loginContext.logout() call: one logout() call at the end of the test inside the try block and another logout() call in the finally block. When the test finishes, the first logout() call removes the kerbClientPrinc KerberosPrincipal in Krb5LoginModule, so when logout() is called for the second time in the finally block, it tries to remove a null kerbClientPrinc at Krb5LoginModule.java:1193: subject.getPrincipals().remove(kerbClientPrinc); where subject is a javax.security.auth.Subject, getPrincipals() returns Set and the Set implementation is a javax.security.auth.Subject.SecureSet. In Java 9, SecureSet's remove() method has introduced a new requireNonNull check for its parameter Object o, which fails if someone tries to remove a null from a SecureSet: Objects.requireNonNull(o,ResourcesMgr.getString(âinvalid.null.input.s.â)); Java 8 (and before) did not have this check in the SecureSet.remove() method, and this is the reason why this NPE appeared in Java 9. Solution: The unit test was fixed by adding an additional condition before running the logout() call in the finally block: logout() is called only if the Set of Principals is not empty i.e. logout() was not already called inside the try block. Note: Inside ZK, LoginContext logout() is called only once in the org.apache.zookeeper.Login reLogin() method, when ZK does a re-login after refreshing the Kerberos tickets. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2908 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/390.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 #390 commit fe08ff5cbf50fdba17714c72416ca4e6a9f4e79a Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-10-03T20:48:44Z ZOOKEEPER-2908: quorum.auth.MiniKdcTest.testKerberosLogin failing with NPE on java 9 ---
[GitHub] zookeeper pull request #375: ZOOKEEPER-1363: Categorise unit tests by 'test-...
GitHub user mfenes reopened a pull request:
https://github.com/apache/zookeeper/pull/375
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
Added new capability of defining test categories in test category
configuration files. Test category configuration files can contain list of test
file names and/or patterns. It is also possible to exclude tests from a test
category. Test categories are added to ${test.src.dir}/category directory.
Current test categories are defined so that they remained API compatible with
the previous build.xml in regards to how âant testâ should be run (options
are the same and they behave the same way).
Func and Perf categories are added to keep the previous FuncTest and
PerfTest categories which were coded into the test file names.
Slow test category is added and contains tests which require more than 30
seconds to complete on a local developer machine.
Command âant test -Dtest.quick=yesâ runs tests minus tests in the Slow
category.
Command âant test -Dtest.category=â runs tests from the
specified category.
It is possible to combine these two options to run a test category minus
the Slow tests.
Test category file name must match the name of the test category (e.g.
category/Commit).
List of excluded tests/patterns should be added to a .exclude file having
the same name as the category (e.g. category/Commit.exclude).
Added README.txt which describes how to run unit tests in detail.
Benefits:
- test categories do not need to be included in the test file names (a
test can be part of more than just one category).
- Slow test category is more flexible to exclude slow tests than the
hardcoded exclusion of â**/*HammerTest.javaâ in build.xml.
- new test categorization can also exclude test file names/patterns from a
category.
- test category can be added for a patch.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-1363
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/375.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 #375
commit cd51fe57cd839f49bf7babdae4ebded3c92f4b26
Author: Mark Fenes <mfe...@cloudera.com>
Date: 2017-09-08T15:00:29Z
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
---
[GitHub] zookeeper pull request #375: ZOOKEEPER-1363: Categorise unit tests by 'test-...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/375 ---
[GitHub] zookeeper pull request #375: ZOOKEEPER-1363: Categorise unit tests by 'test-...
GitHub user mfenes opened a pull request:
https://github.com/apache/zookeeper/pull/375
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
Added new capability of defining test categories in test category
configuration files. Test category configuration files can contain list of test
file names and/or patterns. It is also possible to exclude tests from a test
category. Test categories are added to ${test.src.dir}/category directory.
Current test categories are defined so that they remained API compatible with
the previous build.xml in regards to how âant testâ should be run (options
are the same and they behave the same way).
Func and Perf categories are added to keep the previous FuncTest and
PerfTest categories which were coded into the test file names.
Slow test category is added and contains tests which require more than 30
seconds to complete on a local developer machine.
Command âant test -Dtest.quick=yesâ runs tests minus tests in the Slow
category.
Command âant test -Dtest.category=â runs tests from the
specified category.
It is possible to combine these two options to run a test category minus
the Slow tests.
Test category file name must match the name of the test category (e.g.
category/Commit).
List of excluded tests/patterns should be added to a .exclude file having
the same name as the category (e.g. category/Commit.exclude).
Added README.txt which describes how to run unit tests in detail.
Benefits:
- test categories do not need to be included in the test file names (a
test can be part of more than just one category).
- Slow test category is more flexible to exclude slow tests than the
hardcoded exclusion of â**/*HammerTest.javaâ in build.xml.
- new test categorization can also exclude test file names/patterns from a
category.
- test category can be added for a patch.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-1363
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/375.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 #375
commit cd51fe57cd839f49bf7babdae4ebded3c92f4b26
Author: Mark Fenes <mfe...@cloudera.com>
Date: 2017-09-08T15:00:29Z
ZOOKEEPER-1363: Categorise unit tests by 'test-commit', 'full-test' etc
---
[GitHub] zookeeper pull request #355: ZOOKEEPER-2809: Unnecessary stack-trace in serv...
GitHub user mfenes reopened a pull request: https://github.com/apache/zookeeper/pull/355 ZOOKEEPER-2809: Unnecessary stack-trace in server when the client dis⦠Unnecessary stack-trace in server when the client disconnects unexpectedly. Backport from master, branch-3.5 to branch-3.4. Removes unnecessary stack traces from the catch blocks of method doIO in NIOServerCnxn. For EndOfStreamException stack trace is replaced with logging only the message and also contains the removal of stack traces for exceptions CancelledKeyException and IOException as per commit 6206b495 referenced in the ticket. This change is necessary as there are projects which consider all stack traces as bugs. For CancelledKeyException and IOException developers are still able to see stack traces at log level Debug. This change is in sync with master and branch-3.5. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2809 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/355.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 #355 ---
[GitHub] zookeeper pull request #355: ZOOKEEPER-2809: Unnecessary stack-trace in serv...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/355 ---
[GitHub] zookeeper pull request #355: ZOOKEEPER-2809: Unnecessary stack-trace in serv...
GitHub user mfenes reopened a pull request: https://github.com/apache/zookeeper/pull/355 ZOOKEEPER-2809: Unnecessary stack-trace in server when the client dis⦠Unnecessary stack-trace in server when the client disconnects unexpectedly. Backport from master, branch-3.5 to branch-3.4. Removes unnecessary stack traces from the catch blocks of method doIO in NIOServerCnxn. For EndOfStreamException stack trace is replaced with logging only the message and also contains the removal of stack traces for exceptions CancelledKeyException and IOException as per commit 6206b495 referenced in the ticket. This change is necessary as there are projects which consider all stack traces as bugs. For CancelledKeyException and IOException developers are still able to see stack traces at log level Debug. This change is in sync with master and branch-3.5. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2809 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/355.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 #355 ---
[GitHub] zookeeper pull request #355: ZOOKEEPER-2809: Unnecessary stack-trace in serv...
Github user mfenes closed the pull request at: https://github.com/apache/zookeeper/pull/355 ---
[GitHub] zookeeper pull request #355: ZOOKEEPER-2809: Unnecessary stack-trace in serv...
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/355#discussion_r137005402
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -374,14 +373,12 @@ void doIO(SelectionKey k) throws InterruptedException
{
// expecting close to log session closure
close();
} catch (EndOfStreamException e) {
-LOG.warn("caught end of stream exception",e); // tell user why
-
+LOG.warn(e.getMessage());
--- End diff --
Yes, it would make sense to log the stack trace at debug level for
EndOfStreamException too, so that we don't get less information in the log
after this change is applied. The reason why I did not include it first, was to
keep this change a pure backport from master and branch-3.5 to branch-3.4 as
the original change does not include this additional logging statement. I've
added it now, but probably then we should also add it to master as well if we
wish to keep it.
---
[GitHub] zookeeper pull request #355: ZOOKEEPER-2809: Unnecessary stack-trace in serv...
GitHub user mfenes opened a pull request: https://github.com/apache/zookeeper/pull/355 ZOOKEEPER-2809: Unnecessary stack-trace in server when the client dis⦠Unnecessary stack-trace in server when the client disconnects unexpectedly. Backport from master, branch-3.5 to branch-3.4. Removes unnecessary stack traces from the catch blocks of method doIO in NIOServerCnxn. For EndOfStreamException stack trace is replaced with logging only the message and also contains the removal of stack traces for exceptions CancelledKeyException and IOException as per commit 6206b495 referenced in the ticket. This change is necessary as there are projects which consider all stack traces as bugs. For CancelledKeyException and IOException developers are still able to see stack traces at log level Debug. This change is in sync with master and branch-3.5. You can merge this pull request into a Git repository by running: $ git pull https://github.com/mfenes/zookeeper ZOOKEEPER-2809 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/355.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 #355 commit 81cd3cc42d85371bd24e427bedab1740695be819 Author: Mark Fenes <mfe...@cloudera.com> Date: 2017-08-31T12:11:09Z ZOOKEEPER-2809: Unnecessary stack-trace in server when the client disconnect unexpectedly --- 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. ---
