I think we do. In particular when we check for corruption we base the index check on the file size of the data files so we need to ensure the file metadata (where the size is maintained) is in sync. Otherwise we can drop messages from the index due to them being outside the reported file size. In general, we can only depend on the behaviour described by the java api rather than any particular underlying os call used by a particular implementation. I think this change needs a revert.
On 30 December 2013 15:51, Hiram Chirino <[email protected]> wrote: > If all calls get switched to 'getChannel().force(false)' don't we run > the risk of loosing data on recovery due to the file size not being > consistent? File size is part of the meta-data right? > > On Thu, Dec 19, 2013 at 6:36 PM, <[email protected]> wrote: >> Updated Branches: >> refs/heads/trunk 7656e8262 -> c50b6c39b >> >> >> Fix for: https://issues.apache.org/jira/browse/AMQ-4947 >> >> Project: http://git-wip-us.apache.org/repos/asf/activemq/repo >> Commit: http://git-wip-us.apache.org/repos/asf/activemq/commit/c50b6c39 >> Tree: http://git-wip-us.apache.org/repos/asf/activemq/tree/c50b6c39 >> Diff: http://git-wip-us.apache.org/repos/asf/activemq/diff/c50b6c39 >> >> Branch: refs/heads/trunk >> Commit: c50b6c39babb9e0c4265e632aabf9c38f69c6730 >> Parents: 7656e82 >> Author: Timothy Bish <[email protected]> >> Authored: Thu Dec 19 18:36:05 2013 -0500 >> Committer: Timothy Bish <[email protected]> >> Committed: Thu Dec 19 18:36:05 2013 -0500 >> >> ---------------------------------------------------------------------- >> .../journal/CallerBufferingDataFileAppender.java | 2 +- >> .../kahadb/disk/journal/DataFileAccessor.java | 2 +- >> .../kahadb/disk/journal/DataFileAppender.java | 2 +- >> .../activemq/store/kahadb/disk/page/PageFile.java | 10 +++++----- >> .../store/kahadb/disk/util/DiskBenchmark.java | 6 +++--- >> .../util/RecoverableRandomAccessFile.java | 18 +++++++++++++++++- >> 6 files changed, 28 insertions(+), 12 deletions(-) >> ---------------------------------------------------------------------- >> >> >> http://git-wip-us.apache.org/repos/asf/activemq/blob/c50b6c39/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/CallerBufferingDataFileAppender.java >> ---------------------------------------------------------------------- >> diff --git >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/CallerBufferingDataFileAppender.java >> >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/CallerBufferingDataFileAppender.java >> index ff11848..a6cce59 100644 >> --- >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/CallerBufferingDataFileAppender.java >> +++ >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/CallerBufferingDataFileAppender.java >> @@ -155,7 +155,7 @@ class CallerBufferingDataFileAppender extends >> DataFileAppender { >> } >> >> if (forceToDisk) { >> - file.getFD().sync(); >> + file.getChannel().force(false); >> } >> >> Journal.WriteCommand lastWrite = wb.writes.getTail(); >> >> http://git-wip-us.apache.org/repos/asf/activemq/blob/c50b6c39/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAccessor.java >> ---------------------------------------------------------------------- >> diff --git >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAccessor.java >> >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAccessor.java >> index 7781b7e..11a99dc 100644 >> --- >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAccessor.java >> +++ >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAccessor.java >> @@ -155,7 +155,7 @@ final class DataFileAccessor { >> int size = Math.min(data.getLength(), location.getSize()); >> file.write(data.getData(), data.getOffset(), size); >> if (sync) { >> - file.getFD().sync(); >> + file.getChannel().force(false); >> } >> >> } >> >> http://git-wip-us.apache.org/repos/asf/activemq/blob/c50b6c39/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAppender.java >> ---------------------------------------------------------------------- >> diff --git >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAppender.java >> >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAppender.java >> index 095db52..5f73d2a 100644 >> --- >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAppender.java >> +++ >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/journal/DataFileAppender.java >> @@ -365,7 +365,7 @@ class DataFileAppender implements FileAppender { >> } >> >> if (forceToDisk) { >> - file.getFD().sync(); >> + file.getChannel().force(false); >> } >> >> Journal.WriteCommand lastWrite = wb.writes.getTail(); >> >> http://git-wip-us.apache.org/repos/asf/activemq/blob/c50b6c39/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java >> ---------------------------------------------------------------------- >> diff --git >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java >> >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java >> index 3f107a6..508f698 100644 >> --- >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java >> +++ >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/page/PageFile.java >> @@ -610,10 +610,10 @@ public class PageFile { >> // So we don't loose it.. write it 2 times... >> writeFile.seek(0); >> writeFile.write(d); >> - writeFile.getFD().sync(); >> + writeFile.getChannel().force(false); >> writeFile.seek(PAGE_FILE_HEADER_SIZE / 2); >> writeFile.write(d); >> - writeFile.getFD().sync(); >> + writeFile.getChannel().force(false); >> } >> >> private void storeFreeList() throws IOException { >> @@ -1081,9 +1081,9 @@ public class PageFile { >> if (enableDiskSyncs) { >> // Sync to make sure recovery buffer writes land on disk.. >> if (enableRecoveryFile) { >> - recoveryFile.getFD().sync(); >> + recoveryFile.getChannel().force(false); >> } >> - writeFile.getFD().sync(); >> + writeFile.getChannel().force(false); >> } >> } finally { >> synchronized (writes) { >> @@ -1185,7 +1185,7 @@ public class PageFile { >> } >> >> // And sync it to disk >> - writeFile.getFD().sync(); >> + writeFile.getChannel().force(false); >> return nextTxId; >> } >> >> >> http://git-wip-us.apache.org/repos/asf/activemq/blob/c50b6c39/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/util/DiskBenchmark.java >> ---------------------------------------------------------------------- >> diff --git >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/util/DiskBenchmark.java >> >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/util/DiskBenchmark.java >> index 2805f5f..4615fed 100644 >> --- >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/util/DiskBenchmark.java >> +++ >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/disk/util/DiskBenchmark.java >> @@ -233,9 +233,9 @@ public class DiskBenchmark { >> } >> // Sync to disk so that the we actually write the data to >> disk.. otherwise >> // OS buffering might not really do the write. >> - raf.getFD().sync(); >> + raf.getChannel().force(false); >> } >> - raf.getFD().sync(); >> + raf.getChannel().force(false); >> raf.close(); >> now = System.currentTimeMillis(); >> >> @@ -254,7 +254,7 @@ public class DiskBenchmark { >> for( long i=0; i+data.length < size; i+=data.length) { >> raf.seek(i); >> raf.write(data); >> - raf.getFD().sync(); >> + raf.getChannel().force(false); >> ioCount++; >> now = System.currentTimeMillis(); >> if( (now-start)>sampleInterval ) { >> >> http://git-wip-us.apache.org/repos/asf/activemq/blob/c50b6c39/activemq-kahadb-store/src/main/java/org/apache/activemq/util/RecoverableRandomAccessFile.java >> ---------------------------------------------------------------------- >> diff --git >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/util/RecoverableRandomAccessFile.java >> >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/util/RecoverableRandomAccessFile.java >> index 35c1586..5411ca0 100644 >> --- >> a/activemq-kahadb-store/src/main/java/org/apache/activemq/util/RecoverableRandomAccessFile.java >> +++ >> b/activemq-kahadb-store/src/main/java/org/apache/activemq/util/RecoverableRandomAccessFile.java >> @@ -16,7 +16,12 @@ >> */ >> package org.apache.activemq.util; >> >> -import java.io.*; >> +import java.io.File; >> +import java.io.FileDescriptor; >> +import java.io.FileNotFoundException; >> +import java.io.IOException; >> +import java.io.RandomAccessFile; >> +import java.nio.channels.FileChannel; >> >> public class RecoverableRandomAccessFile implements java.io.DataOutput, >> java.io.DataInput, java.io.Closeable { >> >> @@ -388,6 +393,17 @@ public class RecoverableRandomAccessFile implements >> java.io.DataOutput, java.io. >> throw ioe; >> } >> } >> + >> + public FileChannel getChannel() throws IOException { >> + >> + try { >> + return getRaf().getChannel(); >> + } catch (IOException ioe) >> + { >> + handleException(); >> + throw ioe; >> + } >> + } >> >> public int read(byte[] b, int off, int len) throws IOException { >> try { >> > > > > -- > Hiram Chirino > > Engineering | Red Hat, Inc. > > [email protected] | fusesource.com | redhat.com > > skype: hiramchirino | twitter: @hiramchirino > > blog: Hiram Chirino's Bit Mojo -- http://redhat.com http://blog.garytully.com
