BROOKLYN-144: avoid err when persisting ha-record - Adds Exception.propagate(msg, throwable) so can include info about the file that could not be created. - Fix FileUtil.setFilePermissions so that createNewFile creates the parent directory if necessary. - HighAvailabilityManagerâs polling task: log.err on first exception, then log.debug for subsequent consecutive exceptions.
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/315babf5 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/315babf5 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/315babf5 Branch: refs/heads/master Commit: 315babf59cdf7206c7f8aac129a3454e5d82c4b6 Parents: 9460544 Author: Aled Sage <[email protected]> Authored: Thu Nov 12 20:42:39 2015 +0000 Committer: Aled Sage <[email protected]> Committed: Thu Nov 12 20:42:39 2015 +0000 ---------------------------------------------------------------------- .../mgmt/ha/HighAvailabilityManagerImpl.java | 17 ++++++--- .../persist/FileBasedStoreObjectAccessor.java | 10 +++--- .../FileBasedStoreObjectAccessorWriterTest.java | 38 ++++++++++++++++++-- .../brooklyn/util/exceptions/Exceptions.java | 29 ++++++++++++--- .../org/apache/brooklyn/util/io/FileUtil.java | 18 +++++----- .../util/exceptions/ExceptionsTest.java | 22 ++++++++++++ 6 files changed, 108 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/315babf5/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerImpl.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerImpl.java index 0d8de30..d6a3efa 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerImpl.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerImpl.java @@ -540,12 +540,20 @@ public class HighAvailabilityManagerImpl implements HighAvailabilityManager { @SuppressWarnings("unchecked") protected void registerPollTask() { final Runnable job = new Runnable() { + private boolean lastFailed; + @Override public void run() { try { publishAndCheck(false); + lastFailed = false; } catch (Exception e) { if (running) { - LOG.error("Problem in HA-poller: "+e, e); + if (lastFailed) { + if (LOG.isDebugEnabled()) LOG.debug("Recurring problem in HA-poller: "+e, e); + } else { + LOG.error("Problem in HA-poller: "+e, e); + lastFailed = true; + } } else { if (LOG.isDebugEnabled()) LOG.debug("Problem in HA-poller, but no longer running: "+e, e); } @@ -562,8 +570,9 @@ public class HighAvailabilityManagerImpl implements HighAvailabilityManager { } }; - LOG.debug("Registering poll task for "+this+", period "+getPollPeriod()); - if (getPollPeriod().equals(Duration.PRACTICALLY_FOREVER)) { + Duration pollPeriod = getPollPeriod(); + LOG.debug("Registering poll task for "+this+", period "+pollPeriod); + if (pollPeriod.equals(Duration.PRACTICALLY_FOREVER)) { // don't schedule - used for tests // (scheduling fires off one initial task in the background before the delay, // which affects tests that want to know exactly when publishing happens; @@ -571,7 +580,7 @@ public class HighAvailabilityManagerImpl implements HighAvailabilityManager { } else { if (pollingTask!=null) pollingTask.cancel(true); - ScheduledTask task = new ScheduledTask(MutableMap.of("period", getPollPeriod(), "displayName", "scheduled:[HA poller task]"), taskFactory); + ScheduledTask task = new ScheduledTask(MutableMap.of("period", pollPeriod, "displayName", "scheduled:[HA poller task]"), taskFactory); pollingTask = managementContext.getExecutionManager().submit(task); } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/315babf5/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessor.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessor.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessor.java index b0ae877..f061884 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessor.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessor.java @@ -30,7 +30,6 @@ import org.slf4j.LoggerFactory; import com.google.common.base.Charsets; import com.google.common.base.Objects; -import com.google.common.base.Throwables; import com.google.common.io.Files; /** @@ -56,7 +55,7 @@ public class FileBasedStoreObjectAccessor implements PersistenceObjectStore.Stor if (!exists()) return null; return Files.asCharSource(file, Charsets.UTF_8).read(); } catch (IOException e) { - throw Throwables.propagate(e); + throw Exceptions.propagate("Problem reading String contents of file "+file, e); } } @@ -66,7 +65,7 @@ public class FileBasedStoreObjectAccessor implements PersistenceObjectStore.Stor if (!exists()) return null; return Files.asByteSource(file).read(); } catch (IOException e) { - throw Throwables.propagate(e); + throw Exceptions.propagate("Problem reading bytes of file "+file, e); } } @@ -83,12 +82,13 @@ public class FileBasedStoreObjectAccessor implements PersistenceObjectStore.Stor Files.write(val, tmpFile, Charsets.UTF_8); FileBasedObjectStore.moveFile(tmpFile, file); } catch (IOException e) { - throw Exceptions.propagate(e); + throw Exceptions.propagate("Problem writing data to file "+file+" (via temporary file "+tmpFile+")", e); } catch (InterruptedException e) { throw Exceptions.propagate(e); } } + // TODO Should this write to the temporary file? Otherwise we'll risk getting a partial view of the write. @Override public void append(String val) { try { @@ -97,7 +97,7 @@ public class FileBasedStoreObjectAccessor implements PersistenceObjectStore.Stor Files.append(val, file, Charsets.UTF_8); } catch (IOException e) { - throw Exceptions.propagate(e); + throw Exceptions.propagate("Problem appending to file "+file, e); } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/315babf5/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessorWriterTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessorWriterTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessorWriterTest.java index 14ae4b2..f6db2df 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessorWriterTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/FileBasedStoreObjectAccessorWriterTest.java @@ -22,6 +22,7 @@ import static org.testng.Assert.assertEquals; import java.io.File; import java.io.IOException; +import java.util.concurrent.TimeUnit; import org.apache.brooklyn.core.mgmt.persist.PersistenceObjectStore.StoreObjectAccessorWithLock; import org.apache.brooklyn.util.os.Os; @@ -29,6 +30,7 @@ import org.apache.brooklyn.util.time.Duration; import org.testng.annotations.Test; import com.google.common.base.Charsets; +import com.google.common.base.Stopwatch; import com.google.common.collect.ImmutableList; import com.google.common.io.Files; @@ -61,16 +63,46 @@ public class FileBasedStoreObjectAccessorWriterTest extends PersistenceStoreObje FileBasedObjectStoreTest.assertFilePermission600(file); } - @Test(enabled = false) + @Test(groups="Integration") + public void testPutCreatesNewFile() throws Exception { + File nonExistantFile = Os.newTempFile(getClass(), "txt"); + nonExistantFile.delete(); + StoreObjectAccessorLocking accessor = new StoreObjectAccessorLocking(new FileBasedStoreObjectAccessor(nonExistantFile, ".tmp")); + try { + accessor.put("abc"); + assertEquals(Files.readLines(nonExistantFile, Charsets.UTF_8), ImmutableList.of("abc")); + } finally { + accessor.delete(); + } + } + + @Test(groups="Integration") + public void testPutCreatesNewFileAndParentDir() throws Exception { + File nonExistantDir = Os.newTempDir(getClass()); + nonExistantDir.delete(); + File nonExistantFile = new File(nonExistantDir, "file.txt"); + StoreObjectAccessorLocking accessor = new StoreObjectAccessorLocking(new FileBasedStoreObjectAccessor(nonExistantFile, ".tmp")); + try { + accessor.put("abc"); + assertEquals(Files.readLines(nonExistantFile, Charsets.UTF_8), ImmutableList.of("abc")); + } finally { + accessor.delete(); + } + } + + @Test(groups={"Integration", "Acceptance"}) public void testFilePermissionsPerformance() throws Exception { long interval = 10 * 1000; // millis long start = System.currentTimeMillis(); int count = 0; + Stopwatch stopwatch = Stopwatch.createStarted(); while (System.currentTimeMillis() < start + interval) { accessor.put("abc" + count); - ++count; + count++; } - System.out.println("writes per second:" + (count * 1000 / interval)); + stopwatch.stop(); + double writesPerSec = ((double)count) / stopwatch.elapsed(TimeUnit.MILLISECONDS) * 1000; + System.out.println("writes per second: " + writesPerSec); } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/315babf5/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java b/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java index 9439faa..9643018 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java @@ -97,12 +97,31 @@ public class Exceptions { * <li> wraps as PropagatedRuntimeException for easier filtering */ public static RuntimeException propagate(Throwable throwable) { - if (throwable instanceof InterruptedException) + if (throwable instanceof InterruptedException) { throw new RuntimeInterruptedException((InterruptedException) throwable); + } else if (throwable instanceof RuntimeInterruptedException) { + Thread.currentThread().interrupt(); + throw (RuntimeInterruptedException) throwable; + } Throwables.propagateIfPossible(checkNotNull(throwable)); throw new PropagatedRuntimeException(throwable); } + /** + * See {@link #propagate(Throwable)}. If wrapping the exception, then include the given message; + * otherwise the message is not used. + */ + public static RuntimeException propagate(String msg, Throwable throwable) { + if (throwable instanceof InterruptedException) { + throw new RuntimeInterruptedException(msg, (InterruptedException) throwable); + } else if (throwable instanceof RuntimeInterruptedException) { + Thread.currentThread().interrupt(); + throw (RuntimeInterruptedException) throwable; + } + Throwables.propagateIfPossible(checkNotNull(throwable)); + throw new PropagatedRuntimeException(msg, throwable); + } + /** * Propagate exceptions which are fatal. * <p> @@ -110,12 +129,14 @@ public class Exceptions { * such as {@link InterruptedException} and {@link Error}s. */ public static void propagateIfFatal(Throwable throwable) { - if (throwable instanceof InterruptedException) + if (throwable instanceof InterruptedException) { throw new RuntimeInterruptedException((InterruptedException) throwable); - if (throwable instanceof RuntimeInterruptedException) + } else if (throwable instanceof RuntimeInterruptedException) { + Thread.currentThread().interrupt(); throw (RuntimeInterruptedException) throwable; - if (throwable instanceof Error) + } else if (throwable instanceof Error) { throw (Error) throwable; + } } /** returns the first exception of the given type, or null */ http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/315babf5/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java b/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java index 45bea31..a4908f2 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java @@ -26,11 +26,9 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.List; -import java.util.regex.Pattern; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; -import org.apache.brooklyn.util.io.FileUtil; import org.apache.brooklyn.util.os.Os; import org.apache.brooklyn.util.stream.StreamGobbler; import org.apache.brooklyn.util.stream.Streams; @@ -40,12 +38,6 @@ import org.slf4j.LoggerFactory; import com.google.common.annotations.Beta; import com.google.common.collect.ImmutableList; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.attribute.PosixFileAttributeView; -import java.nio.file.attribute.PosixFilePermission; -import java.nio.file.attribute.PosixFilePermissions; -import java.util.Set; public class FileUtil { @@ -57,7 +49,7 @@ public class FileUtil { private static final FilePermissions permissions700 = new FilePermissions(0700); public static void setFilePermissionsTo700(File file) throws IOException { - file.createNewFile(); + createNewFile(file); try { permissions700.apply(file); if (LOG.isTraceEnabled()) LOG.trace("Set permissions to 700 for file {}", file.getAbsolutePath()); @@ -67,7 +59,7 @@ public class FileUtil { } public static void setFilePermissionsTo600(File file) throws IOException { - file.createNewFile(); + createNewFile(file); try { permissions600.apply(file); if (LOG.isTraceEnabled()) LOG.trace("Set permissions to 600 for file {}", file.getAbsolutePath()); @@ -186,4 +178,10 @@ public class FileUtil { } } + private static boolean createNewFile(File file) throws IOException { + if (!file.getParentFile().exists()) { + file.getParentFile().mkdirs(); + } + return file.createNewFile(); + } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/315babf5/utils/common/src/test/java/org/apache/brooklyn/util/exceptions/ExceptionsTest.java ---------------------------------------------------------------------- diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/exceptions/ExceptionsTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/exceptions/ExceptionsTest.java index 0e7d590..dee7651 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/exceptions/ExceptionsTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/exceptions/ExceptionsTest.java @@ -59,6 +59,28 @@ public class ExceptionsTest { } @Test + public void testPropagateCheckedExceptionWithMessage() throws Exception { + String extraMsg = "my message"; + Exception tothrow = new Exception("simulated"); + try { + throw Exceptions.propagate(extraMsg, tothrow); + } catch (RuntimeException e) { + assertEquals(e.getMessage(), "my message"); + assertEquals(e.getCause(), tothrow); + } + } + + @Test + public void testPropagateRuntimeExceptionIgnoresMessage() throws Exception { + NullPointerException tothrow = new NullPointerException("simulated"); + try { + throw Exceptions.propagate("my message", tothrow); + } catch (NullPointerException e) { + assertEquals(e, tothrow); + } + } + + @Test public void testPropagateIfFatalPropagatesInterruptedException() throws Exception { InterruptedException tothrow = new InterruptedException("simulated"); try {
