You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/01/29 16:58:31 UTC
[GitHub] [bookkeeper] atris opened a new pull request #2257: 2176:
IOException Should Be Thrown When Journal Is Corrupted
atris opened a new pull request #2257: 2176: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-589950869
LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2257: 2176: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2257: 2176: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-579878383
Yes it can be a good place to put the test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r380811767
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,50 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
Review comment:
This annotation works at a class level -- not sure how I can make it to the specific line?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-587569809
@eolivelli Updated, please see and let me know your thoughts
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r378672533
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,50 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
Review comment:
Is is too ambiguous.
We ate not sure that only the last line threw this exception.
Please move the assertion exactly around the expected invocation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli merged pull request #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r378672119
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,50 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = new Bookie(conf);
+ for (Journal journal : b.journals) {
+ List<Long> journalIds = journal.listJournalIds(journal.getJournalDirectory(), null);
+
+ assert journalIds.size() == 1;
Review comment:
Use assertEquals
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-589935322
@eolivelli Updated, please see and let me know your thoughts
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r382898753
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,53 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = null;
+
+ try {
+ b = new Bookie(conf);
+ } finally {
+ b.shutdown();
+ }
+
+ for (Journal journal : b.journals) {
+ List<Long> journalIds = journal.listJournalIds(journal.getJournalDirectory(), null);
+
+ assertEquals(journalIds.size(), 1);
+
+ journal.scanJournal(journalIds.get(0), Long.MAX_VALUE, journalScanner);
Review comment:
I tried that but it seems too brittle a check for depending on the error message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r378672666
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,50 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = new Bookie(conf);
Review comment:
Shall we stop the Bookie in a finally block?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r382906205
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,63 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ /**
+ * Test for fake IOException during read of Journal.
+ */
+ @Test
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = null;
+
+ try {
+ b = new Bookie(conf);
+ } finally {
+ b.shutdown();
Review comment:
Oops, sorry, slipped through the cracks. Updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-589947530
@eolivelli Updated, please see. On another note, I see that TestEntryLog has just started failing again (the PR checks passed on my last commit 2 hours ago). Did something change?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r378417227
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java
##########
@@ -160,7 +176,13 @@ private JournalChannel(File journalDirectory, long logId,
+ " suddenly appeared, is another bookie process running?");
}
randomAccessFile = new RandomAccessFile(fn, "rw");
- fc = randomAccessFile.getChannel();
+
+ if (dummyFC == null) {
+ fc = randomAccessFile.getChannel();
Review comment:
@eolivelli Fixed, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-584801825
The issue there would be that the RandomFileAccess instance used to create
the FileChannel is also created in the same context so we will probably
have to powermock up the call stack.
I felt this way is a longer term way to test different cases of FileChannel
implementations especially since FileChannel is an external class not owned
by our codebase.
Thoughts?
--
Regards,
Atri
*l'apprenant*
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-594720694
Can we merge now?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2176: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2176: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-579870105
> Can you please referente the originale issue in the description of this pr.
Done
> Also we need a test that demonstrate the fix and an explanation about how it works
I wanted to check on that. Looking in BookieJournalTest, can I use writeV5Journal API to construct a corrupted file and test that the bookie closes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-590193121
@atris please rebase to latest master, we have fixed a problem with git checkout
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-595633930
Merged to master branch, thank you @atris
sorry that it took so much time.
I hope that our CI will be in better shape soon
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] karanmehta93 commented on issue #2257: 2178:
IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
karanmehta93 commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-594783403
Thank you @atris for fixing this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r380837649
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,50 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
Review comment:
It is not at **class** level, but it works around this **method**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-584781266
@sijie @eolivelli Updated the PR, please see and comment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r377818469
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java
##########
@@ -160,7 +176,13 @@ private JournalChannel(File journalDirectory, long logId,
+ " suddenly appeared, is another bookie process running?");
}
randomAccessFile = new RandomAccessFile(fn, "rw");
- fc = randomAccessFile.getChannel();
+
+ if (dummyFC == null) {
+ fc = randomAccessFile.getChannel();
Review comment:
What about using Powermock in order to hack the result of getChannel ?
I feel this way of testing is quite invasive
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r378671918
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,50 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = new Bookie(conf);
+ for (Journal journal : b.journals) {
+ List<Long> journalIds = journal.listJournalIds(journal.getJournalDirectory(), null);
+
+ assert journalIds.size() == 1;
+
+ journal.scanJournal(journalIds.get(0), Long.MAX_VALUE, journalScanner);
+ }
+ }
+
+ private class DummyJournalScan implements Journal.JournalScanner {
+ boolean printJournalVersion = false;
+
+ @Override
+ public void process(int journalVersion, long offset, ByteBuffer entry) throws IOException {
+ if (!printJournalVersion) {
+ System.out.println("Journal Version : " + journalVersion);
Review comment:
Use Logger please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] sijie commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
sijie commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-583680957
@atris I think the issue was thrown when attempting to seek to a position doesn't exist. so I guess you can truncate the file to reproduce.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-583752881
> @atris I think the issue was thrown when attempting to seek to a position doesn't exist. so I guess you can truncate the file to reproduce.
@sijie Thanks, I updated the test but FileChannel.position then moves the file pointer to the asked location. What am I missing, please?
https://gist.github.com/atris/773e1074edd1d50786ff35e300495984
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r378672211
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java
##########
@@ -290,4 +291,12 @@ public void forceWrite(boolean forceMetadata) throws IOException {
this.lastDropPosition = newDropPos;
}
}
+
+ public static FileChannel openFileChannel(RandomAccessFile randomAccessFile) {
Review comment:
Add VisibleForTesting comment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris edited a comment on issue #2257: 2176:
IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris edited a comment on issue #2257: 2176: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-579870105
> Can you please referente the originale issue in the description of this pr.
Done
> Also we need a test that demonstrate the fix and an explanation about how it works
I wanted to check on that. Looking in BookieJournalTest, can I use writeV5Journal API to construct a corrupted file and test that the bookie closes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r377855884
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java
##########
@@ -160,7 +176,13 @@ private JournalChannel(File journalDirectory, long logId,
+ " suddenly appeared, is another bookie process running?");
}
randomAccessFile = new RandomAccessFile(fn, "rw");
- fc = randomAccessFile.getChannel();
+
+ if (dummyFC == null) {
+ fc = randomAccessFile.getChannel();
Review comment:
You can add a static method like:
FileChannel openFileChannel (RandomAccessFile)
Then you can override it with PowerMock
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r380837875
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,53 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = null;
+
+ try {
+ b = new Bookie(conf);
+ } finally {
Review comment:
you should shut down the bookie after the " for (Journal journal : b.journals) {" loop
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-590191979
@sijie do you have comments?
@merlimat you may be interested in this fix
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r382902088
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,63 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ /**
+ * Test for fake IOException during read of Journal.
+ */
+ @Test
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = null;
+
+ try {
+ b = new Bookie(conf);
+ } finally {
+ b.shutdown();
Review comment:
Why are you shutting down the bookie twice?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r380838764
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,53 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = null;
+
+ try {
+ b = new Bookie(conf);
+ } finally {
+ b.shutdown();
+ }
+
+ for (Journal journal : b.journals) {
+ List<Long> journalIds = journal.listJournalIds(journal.getJournalDirectory(), null);
+
+ assertEquals(journalIds.size(), 1);
+
+ journal.scanJournal(journalIds.get(0), Long.MAX_VALUE, journalScanner);
Review comment:
try {
journal.scanJournal(journalIds.get(0), Long.MAX_VALUE, journalScanner) ;
fail("should not be able to scan the journal")
} catch (IOException err) {
// expected
// maybe you can perform some assertion about the err.getMessage() result, but maybe it depends on java version or locale
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-594704835
Hopefully we can commit this patch
Pinging @sijie @ivankelly @jiazhai
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-583528199
@eolivelli Need a bit of help with the test. I have created a test such as:
https://gist.github.com/atris/773e1074edd1d50786ff35e300495984
However, I was not able to trigger the IOException that this PR added. What am I doing wrong, please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-593617625
@atris please rebase to latest master, we have updated the github action "checkout"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2257:
2178: IOException Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#discussion_r378672837
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
##########
@@ -794,4 +801,50 @@ private void testPartialFileInfoPostV3Journal(boolean truncateMasterKey)
// correct behaviour
}
}
+
+ @Test(expected = IOException.class)
+ public void testJournalScanIOException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ writeV4Journal(Bookie.getCurrentDirectory(journalDir), 100, "testPasswd".getBytes());
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath() })
+ .setMetadataServiceUri(null);
+
+ Journal.JournalScanner journalScanner = new DummyJournalScan();
+ FileChannel fileChannel = PowerMockito.mock(FileChannel.class);
+
+ PowerMockito.when(fileChannel.position(Mockito.anyLong()))
+ .thenThrow(new IOException());
+
+ PowerMockito.mockStatic(JournalChannel.class);
+ PowerMockito.when(JournalChannel.openFileChannel(Mockito.any(RandomAccessFile.class))).thenReturn(fileChannel);
+
+ Bookie b = new Bookie(conf);
+ for (Journal journal : b.journals) {
+ List<Long> journalIds = journal.listJournalIds(journal.getJournalDirectory(), null);
+
+ assert journalIds.size() == 1;
+
+ journal.scanJournal(journalIds.get(0), Long.MAX_VALUE, journalScanner);
+ }
+ }
+
+ private class DummyJournalScan implements Journal.JournalScanner {
+ boolean printJournalVersion = false;
Review comment:
This flag looks useless
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [bookkeeper] atris commented on issue #2257: 2178: IOException
Should Be Thrown When Journal Is Corrupted
Posted by GitBox <gi...@apache.org>.
atris commented on issue #2257: 2178: IOException Should Be Thrown When Journal Is Corrupted
URL: https://github.com/apache/bookkeeper/pull/2257#issuecomment-594695934
@eolivelli DOne
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services