[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[mfenes]

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 #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[eolivelli]

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

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

Maybe it could be a good improvement to start using junit TemporaryFolder 
rule so that the clean up will ve automatically handled by junit


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[mfenes]

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 pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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 pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[afine]

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

https://github.com/apache/zookeeper/pull/450#discussion_r163395510
  
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/Util.java 
---
@@ -50,6 +50,9 @@
 private static final String SNAP_DIR="snapDir";
 private static final String LOG_DIR="logDir";
 private static final String DB_FORMAT_CONV="dbFormatConversion";
+
+private static final String LOG_FILE_PREFIX = "log";
--- End diff --

Can we use these field when actually creating the log and snapshot files?


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[afine]

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

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

Could we add a reference to the discussion that explains why this whole 
check is needed in the comments and/or the exception that the user sees?


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[afine]

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

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

there is a lot of code duplication in these tests, i'm wondering if they 
can be cleaned up.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

Okay.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

I prefer writing (and later reading) less code than more. You could be 
right about the performance impact, but at least `listFiles()` will only create 
File objects for the matching files, not for all of them.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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

[mfenes]

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 pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

This is always true.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

Again, I believe these checks are redundant. createNewFile() will always 
successfully create the file or throws exception.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

What do you think of using one of the filtered version of listFiles() 
rather than getting all the files in the directory?
For example, using with FilenameFilter:

https://docs.oracle.com/javase/7/docs/api/java/io/File.html#listFiles(java.io.FilenameFilter)


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

Same here with the dot.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

https://github.com/apache/zookeeper/pull/450#discussion_r162945403
  
--- 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 wonder if LOG_FILE_PREFIX included the dot, would be better for the 
validation.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

These assert will never fail. mkdirs() throws exception if the directory 
cannot be created.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

It might be safer to check here with the dot (".") included in the prefix.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

Setting 'zookeeper.datadir.autocreate' doesn't make any difference here, so 
it doesn't need to be maintained.
In addition to that please add clean-up logic to your tests. (e.g. 
recursively remove temp directories with all their contents)


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

These 2 catches can be grouped by catching all Exceptions.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

https://github.com/apache/zookeeper/pull/450#discussion_r162954011
  
--- 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);
+}
+}
+}
+
+@Test
+public void testDirCheckWithSameLogAndSnapDirs() throws IOException {
--- End diff --

I don't repeat my previous comments, all of them applies to the rest of 
test cases.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

Same here. Get files with fileNameFilter and if it returns non-empty array, 
throw exception.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[anmolnar]

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

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

This is always true.


---

[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...

[mfenes]

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 
Date:   2018-01-19T23:16:07Z

ZOOKEEPER-2967: Add check to validate dataDir and dataLogDir parameters at 
startup




---