[
https://issues.apache.org/jira/browse/QPID-8770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18122008#comment-18122008
]
ASF GitHub Bot commented on QPID-8770:
--------------------------------------
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.
> [Broker-J] FileLogger does not roll on restart when daily rolling is enabled
> ----------------------------------------------------------------------------
>
> Key: QPID-8770
> URL: https://issues.apache.org/jira/browse/QPID-8770
> Project: Qpid
> Issue Type: Bug
> Components: Broker-J
> Affects Versions: qpid-java-broker-10.1.0, qpid-java-broker-10.1.1
> Reporter: Takahashi Kiyoshi
> Priority: Minor
>
> When both rollDaily and rollOnRestart are enabled, restarting the broker on
> the same day can append to the existing log instead of rolling it over.
> Expected behavior: archive the existing nonempty log before writing the first
> new log event, using the daily archive filename pattern.
> This appears to be a regression of QPID-8081, introduced during the Logback
> upgrade in QPID-8626.
> The fix should replace the daily triggering policy to restore restart
> rollover, preserve archive numbering, and avoid archiving empty files.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
