dakirily commented on code in PR #451: URL: https://github.com/apache/qpid-broker-j/pull/451#discussion_r4167158361
########## broker-plugins/logging-logback/src/main/java/org/apache/qpid/server/logging/logback/DailyTriggeringPolicy.java: ########## @@ -0,0 +1,281 @@ +/* + * + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + */ +package org.apache.qpid.server.logging.logback; + +import java.io.File; +import java.io.IOException; +import java.nio.file.DirectoryIteratorException; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.nio.file.attribute.BasicFileAttributes; +import java.time.Instant; +import java.util.Locale; +import java.util.Objects; +import java.util.TimeZone; +import java.util.concurrent.TimeUnit; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import ch.qos.logback.core.rolling.LengthCounter; +import ch.qos.logback.core.rolling.LengthCounterBase; +import ch.qos.logback.core.rolling.TimeBasedFileNamingAndTriggeringPolicy; +import ch.qos.logback.core.rolling.TimeBasedRollingPolicy; +import ch.qos.logback.core.rolling.helper.ArchiveRemover; +import ch.qos.logback.core.rolling.helper.Compressor; +import ch.qos.logback.core.rolling.helper.DateTokenConverter; +import ch.qos.logback.core.rolling.helper.FileNamePattern; +import ch.qos.logback.core.rolling.helper.RollingCalendar; +import ch.qos.logback.core.rolling.helper.SizeAndTimeBasedArchiveRemover; +import ch.qos.logback.core.spi.ContextAwareBase; +import ch.qos.logback.core.util.FileSize; + +final class DailyTriggeringPolicy<E> extends ContextAwareBase implements TimeBasedFileNamingAndTriggeringPolicy<E> +{ + private static final long RETRY_DELAY_MILLIS = TimeUnit.SECONDS.toMillis(30); + + private final FileSize _maxFileSize; + private final boolean _rollOnRestart; + private final LengthCounter _lengthCounter = new LengthCounterBase(); + private TimeBasedRollingPolicy<E> _rollingPolicy; + private FileNamePattern _uncompressedPattern; + private RollingCalendar _calendar; + private ArchiveRemover _archiveRemover; + private String _compressionSuffix; + private Instant _period; + private long _nextCheck; + private long _retryAfter; + private int _nextArchiveIndex; + private String _elapsedFileName; + private boolean _restartPending; + private boolean _started; + private long _currentTime = -1; + + DailyTriggeringPolicy(final FileSize maxFileSize, final boolean rollOnRestart) + { + _maxFileSize = Objects.requireNonNull(maxFileSize, "maxFileSize"); + _rollOnRestart = rollOnRestart; + } + + @Override + public void start() + { + if (_started) + { + return; + } + if (_rollingPolicy == null || _rollingPolicy.getParentsRawFileProperty() == null || _maxFileSize.getSize() <= 0) + { + addError("Daily rolling requires a rolling policy, an active filename and a positive maximum file size"); + return; + } + + final String pattern = _rollingPolicy.getFileNamePattern(); + final FileNamePattern archivePattern = new FileNamePattern(pattern, getContext()); + final String uncompressedPattern = Compressor + .computeFileNameStrWithoutCompSuffix(pattern, _rollingPolicy.getCompressionMode()); + _uncompressedPattern = new FileNamePattern(uncompressedPattern, getContext()); + _compressionSuffix = pattern.substring(uncompressedPattern.length()); + final DateTokenConverter<Object> dateConverter = archivePattern.getPrimaryDateTokenConverter(); + if (dateConverter == null || archivePattern.getIntegerTokenConverter() == null || + !uncompressedPattern.endsWith(".%i")) + { + addError("Daily rolling requires a date token and a final .%i archive index"); + return; + } + _calendar = dateConverter.getZoneId() == null + ? new RollingCalendar(dateConverter.getDatePattern()) + : new RollingCalendar(dateConverter.getDatePattern(), TimeZone.getTimeZone(dateConverter.getZoneId()), + Locale.getDefault()); + if (!_calendar.isCollisionFree()) + { + addError("The archive date pattern does not distinguish successive periods"); + return; + } + + try + { + final BasicFileAttributes attributes = readActiveFileAttributes(); + final boolean nonempty = attributes != null && attributes.size() > 0; + _period = nonempty ? attributes.lastModifiedTime().toInstant() : Instant.ofEpochMilli(getCurrentTime()); + _nextCheck = _calendar.getNextTriggeringDate(_period).toEpochMilli(); + _nextArchiveIndex = nextArchiveIndex(_period); + _restartPending = _rollOnRestart && nonempty; + _elapsedFileName = null; + _retryAfter = 0; + _lengthCounter.reset(); + final SizeAndTimeBasedArchiveRemover remover = + new SizeAndTimeBasedArchiveRemover(archivePattern, (RollingCalendar) _calendar.clone()); + remover.setContext(getContext()); + _archiveRemover = remover; + _started = true; + } + catch (IOException | DirectoryIteratorException | ArithmeticException e) + { + addError("Cannot initialize daily log archives", e); + } + } + + private BasicFileAttributes readActiveFileAttributes() throws IOException + { + try + { + return Files.readAttributes(Path.of(_rollingPolicy.getParentsRawFileProperty()), BasicFileAttributes.class); + } + catch (NoSuchFileException ignore) + { + // A new active file will be opened by RollingFileAppender after this policy has started + return null; + } + } + + @Override + public boolean isTriggeringEvent(final File activeFile, final E event) + { + final long now = getCurrentTime(); + if (!_started || now < _retryAfter) + { + return false; + } + try + { + if (now >= _nextCheck) + { + final Instant nextPeriod = Instant.ofEpochMilli(now); + final int nextIndex = nextArchiveIndex(nextPeriod); + _elapsedFileName = getCurrentPeriodsFileNameWithoutCompressionSuffix(); + _period = nextPeriod; + _nextArchiveIndex = nextIndex; + _nextCheck = _calendar.getNextTriggeringDate(nextPeriod).toEpochMilli(); + _restartPending = false; + _retryAfter = 0; + final boolean nonempty = _lengthCounter.getLength() > 0; + _lengthCounter.reset(); + return nonempty; + } + if (_restartPending || _lengthCounter.getLength() >= _maxFileSize.getSize()) + { + final int nextIndex = Math.incrementExact(_nextArchiveIndex); + _elapsedFileName = getCurrentPeriodsFileNameWithoutCompressionSuffix(); + _nextArchiveIndex = nextIndex; + _restartPending = false; + _retryAfter = 0; + _lengthCounter.reset(); + return true; + } + } + catch (IOException | DirectoryIteratorException | ArithmeticException e) Review Comment: Please set `_retryAfter` before calling `DailyTriggeringPolicy#addError()` in this catch block. `addError()` synchronously notifies `BrokerLoggerStatusListener`, which logs through the same appender. When archive scanning throws an `IOException`, this re-enters `DailyTriggeringPolicy#isTriggeringEvent()` while `_retryAfter` is still zero, repeating the failed scan and error notification. This can recurse until `StackOverflowError`. With the actual listener and `broker.failOnLoggerIOError=false`, a bounded reproduction reached six nested notifications before its safety limit. Moving the assignment before `addError()` reduced this to one notification and preserved backoff and recovery. Please capture whether `_retryAfter == 0`, assign the retry deadline, and then report the error using the captured flag. Add regression coverage with the real listener attached. The existing scan-failure test does not exercise this interaction. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
