gemmellr commented on code in PR #4599:
URL: https://github.com/apache/activemq-artemis/pull/4599#discussion_r1317135417
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java:
##########
@@ -3493,12 +3493,25 @@ public void run() {
recoverStoredBridges();
}
- if (configuration.getMaxDiskUsage() != -1) {
- try {
- injectMonitor(new FileStoreMonitor(getScheduledPool(),
executorFactory.getExecutor(), configuration.getDiskScanPeriod(),
TimeUnit.MILLISECONDS, configuration.getMaxDiskUsage() / 100f,
ioCriticalErrorListener));
- } catch (Exception e) {
- ActiveMQServerLogger.LOGGER.unableToInjectMonitor(e);
+ deployFileStoreMonitor();
+ }
+
+ private void deployFileStoreMonitor() throws Exception {
+ FileStoreMonitor.FileStoreMonitorType fileStoreMonitorType = null;
+ Number referenceValue = null;
+ if (configuration.getMinDiskFree() != -1) {
+ if (configuration.getMaxDiskUsage() != -1) {
+
ActiveMQServerLogger.LOGGER.configParamOverride(FileConfigurationParser.MIN_DISK_FREE,
configuration.getMinDiskFree(), FileConfigurationParser.MAX_DISK_USAGE,
configuration.getMaxDiskUsage());
Review Comment:
This will log a warning, however its clearly documented that minDiskFree
takes precedence, and it is already disabled by default in both code and xml
config, whereas in contrast maxDiskUsage is enabled by default in both code and
xml config. So to avoid a log warning users will *need* to explicitly configure
both of them. I'm not sure thats necessary. At the very least it seems more
like info or lower level than a warning if we keep it logging.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingManagerImpl.java:
##########
@@ -237,31 +237,32 @@ class LocalMonitor implements FileStoreMonitor.Callback {
private final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
@Override
- public void tick(long usableSpace, long totalSpace) {
+ public void tick(long usableSpace, long totalSpace, boolean ok,
FileStoreMonitor.FileStoreMonitorType type) {
diskUsableSpace = usableSpace;
diskTotalSpace = totalSpace;
- if (logger.isTraceEnabled()) {
- logger.trace("Tick:: usable space at {}, total space at {}",
ByteUtil.getHumanReadableByteCount(usableSpace),
ByteUtil.getHumanReadableByteCount(totalSpace));
- }
- }
-
- @Override
- public void over(long usableSpace, long totalSpace) {
- if (!diskFull) {
-
ActiveMQServerLogger.LOGGER.diskBeyondCapacity(ByteUtil.getHumanReadableByteCount(usableSpace),
ByteUtil.getHumanReadableByteCount(totalSpace), String.format("%.1f%%",
FileStoreMonitor.calculateUsage(usableSpace, totalSpace) * 100));
- diskFull = true;
- }
- }
-
- @Override
- public void under(long usableSpace, long totalSpace) {
- final boolean diskFull = PagingManagerImpl.this.diskFull;
- if (diskFull || !blockedStored.isEmpty() ||
!memoryCallback.isEmpty()) {
- if (diskFull) {
-
ActiveMQServerLogger.LOGGER.diskCapacityRestored(ByteUtil.getHumanReadableByteCount(usableSpace),
ByteUtil.getHumanReadableByteCount(totalSpace), String.format("%.1f%%",
FileStoreMonitor.calculateUsage(usableSpace, totalSpace) * 100));
- PagingManagerImpl.this.diskFull = false;
+ logger.trace("Tick:: usable space at {}, total space at {}",
ByteUtil.getHumanReadableByteCount(usableSpace),
ByteUtil.getHumanReadableByteCount(totalSpace));
Review Comment:
Its not exactly a hot path so it wont do great harm, but just to note the
gate being removed was preventing doing the unnecessary
string-creations/formats.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/files/FileStoreMonitorTest.java:
##########
@@ -72,71 +72,117 @@ public void testSimpleTick() throws Exception {
bufferedOutputStream.close();
- final AtomicInteger over = new AtomicInteger(0);
- final AtomicInteger under = new AtomicInteger(0);
+ final AtomicInteger overMaxUsage = new AtomicInteger(0);
+ final AtomicInteger underMaxUsage = new AtomicInteger(0);
final AtomicInteger tick = new AtomicInteger(0);
- FileStoreMonitor.Callback callback = new FileStoreMonitor.Callback() {
- @Override
- public void tick(long usableSpace, long totalSpace) {
- tick.incrementAndGet();
- logger.debug("tick:: usableSpace: {}, totalSpace:{}", usableSpace,
totalSpace);
+ FileStoreMonitor.Callback callback = (usableSpace, totalSpace, ok, type)
-> {
+ tick.incrementAndGet();
+ if (ok) {
+ underMaxUsage.incrementAndGet();
+ } else {
+ overMaxUsage.incrementAndGet();
}
+ logger.debug("tick:: usableSpace: " + usableSpace + ", totalSpace:" +
totalSpace);
+ };
+ FileStoreMonitor storeMonitor = new
FileStoreMonitor(scheduledExecutorService, executorService, 100L,
TimeUnit.MILLISECONDS, 0.999, null,
FileStoreMonitor.FileStoreMonitorType.MaxDiskUsage);
+ storeMonitor.addCallback(callback);
+ storeMonitor.addStore(getTestDirfile());
- @Override
- public void over(long usableSpace, long totalSpace) {
- over.incrementAndGet();
- logger.debug("over:: usableSpace: {}, totalSpace:{}", usableSpace,
totalSpace);
- }
+ storeMonitor.tick();
+
+ Assert.assertEquals(0, overMaxUsage.get());
+ Assert.assertEquals(1, tick.get());
+ Assert.assertEquals(1, underMaxUsage.get());
- @Override
- public void under(long usableSpace, long totalSpace) {
- under.incrementAndGet();
- logger.debug("under:: usableSpace: {}, totalSpace: {}",
usableSpace, totalSpace);
+ storeMonitor.setMaxUsage(0);
+
+ storeMonitor.tick();
+
+ Assert.assertEquals(1, overMaxUsage.get());
+ Assert.assertEquals(2, tick.get());
+ Assert.assertEquals(1, underMaxUsage.get());
+ }
+
+ @Test
+ public void testSimpleTickForMinDiskFree() throws Exception {
+ File garbageFile = new File(getTestDirfile(), "garbage.bin");
+ FileOutputStream garbage = new FileOutputStream(garbageFile);
+ BufferedOutputStream bufferedOutputStream = new
BufferedOutputStream(garbage);
+ PrintStream out = new PrintStream(bufferedOutputStream);
+
+ // This is just to make sure there is at least something on the device.
+ // If the testsuite is running with an empty tempFS, it would return 0
and the assertion would fail.
+ for (int i = 0; i < 100; i++) {
+ out.println("Garbage " + i);
+ }
+
+ bufferedOutputStream.close();
Review Comment:
Could this use try-with-resources to eliminate the need to explicitly close,
and make it safer?
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/server/files/FileStoreMonitorTest.java:
##########
@@ -72,71 +72,117 @@ public void testSimpleTick() throws Exception {
bufferedOutputStream.close();
- final AtomicInteger over = new AtomicInteger(0);
- final AtomicInteger under = new AtomicInteger(0);
+ final AtomicInteger overMaxUsage = new AtomicInteger(0);
+ final AtomicInteger underMaxUsage = new AtomicInteger(0);
final AtomicInteger tick = new AtomicInteger(0);
- FileStoreMonitor.Callback callback = new FileStoreMonitor.Callback() {
- @Override
- public void tick(long usableSpace, long totalSpace) {
- tick.incrementAndGet();
- logger.debug("tick:: usableSpace: {}, totalSpace:{}", usableSpace,
totalSpace);
+ FileStoreMonitor.Callback callback = (usableSpace, totalSpace, ok, type)
-> {
+ tick.incrementAndGet();
+ if (ok) {
+ underMaxUsage.incrementAndGet();
+ } else {
+ overMaxUsage.incrementAndGet();
}
+ logger.debug("tick:: usableSpace: " + usableSpace + ", totalSpace:" +
totalSpace);
+ };
+ FileStoreMonitor storeMonitor = new
FileStoreMonitor(scheduledExecutorService, executorService, 100L,
TimeUnit.MILLISECONDS, 0.999, null,
FileStoreMonitor.FileStoreMonitorType.MaxDiskUsage);
Review Comment:
Importing FileStoreMonitor.FileStoreMonitorType could make things a little
less verbose.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingManagerImpl.java:
##########
@@ -237,31 +237,32 @@ class LocalMonitor implements FileStoreMonitor.Callback {
private final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
@Override
- public void tick(long usableSpace, long totalSpace) {
+ public void tick(long usableSpace, long totalSpace, boolean ok,
FileStoreMonitor.FileStoreMonitorType type) {
Review Comment:
Importing FileStoreMonitor.FileStoreMonitorType and shortening the
types/prefixes here/below would probably help make the method a little more
readable.
Something like 'withinBound' might also read a little nicer than 'ok' (here
and at the caller).
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
