ramanathan1504 commented on code in PR #4319:
URL: https://github.com/apache/logging-log4j2/pull/4319#discussion_r4065363663
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManager.java:
##########
@@ -192,9 +192,14 @@ public final synchronized void flush() {
this.writeInternal(event, layout != null ?
layout.toSerializable(event) : null);
}
} finally {
- this.commitAndClose();
- // not sure if this should be done when writing the events
failed
- this.buffer.clear();
+ try {
+ this.commitAndClose();
+ } finally {
+ // The events were already handed to the database layer,
so they must not be kept
+ // when committing fails: the next flush would send them
again and the buffer
+ // would grow without bound while the failure persists.
Review Comment:
```suggestion
```
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManagerTest.java:
##########
@@ -227,6 +230,34 @@ void testBuffering04() throws Exception {
then(manager).shouldHaveNoMoreInteractions();
}
+ @Test
+ void testBufferedEventsAreDiscardedWhenCommitFails() throws Exception {
+ setUp("name", 10);
+
+ final LogEvent event1 = mock(LogEvent.class);
+ final LogEvent event2 = mock(LogEvent.class);
+
+ when(event1.toImmutable()).thenReturn(mock(LogEvent.class));
+ when(event2.toImmutable()).thenReturn(mock(LogEvent.class));
+
+ manager.startup();
+ manager.write(event1, null);
+ manager.write(event2, null);
+
+ // The first flush fails while committing the transaction, the next
one succeeds.
Review Comment:
```suggestion
```
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/db/AbstractDatabaseManagerTest.java:
##########
@@ -227,6 +230,34 @@ void testBuffering04() throws Exception {
then(manager).shouldHaveNoMoreInteractions();
}
+ @Test
+ void testBufferedEventsAreDiscardedWhenCommitFails() throws Exception {
+ setUp("name", 10);
+
+ final LogEvent event1 = mock(LogEvent.class);
+ final LogEvent event2 = mock(LogEvent.class);
+
+ when(event1.toImmutable()).thenReturn(mock(LogEvent.class));
+ when(event2.toImmutable()).thenReturn(mock(LogEvent.class));
+
+ manager.startup();
+ manager.write(event1, null);
+ manager.write(event2, null);
+
+ // The first flush fails while committing the transaction, the next
one succeeds.
+ doThrow(new DbAppenderLoggingException("Failed to commit the
transaction"))
+ .doReturn(true)
+ .when(manager)
+ .commitAndClose();
+
+ assertThrows(DbAppenderLoggingException.class, manager::flush);
+
+ manager.flush();
+
+ // Events of a transaction that failed to commit must not be sent
again.
Review Comment:
```suggestion
```
##########
src/changelog/.2.x.x/4318_fix_database_appender_buffer_on_failed_commit.xml:
##########
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns="https://logging.apache.org/xml/ns";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="
+ https://logging.apache.org/xml/ns
+ https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="fixed">
+ <issue id="4318"
link="https://github.com/apache/logging-log4j2/issues/4318"/>
+ <description format="asciidoc">
+ Clear the buffer of a buffered database appender when committing a
transaction fails.
+ Previously the events were kept, so the next flush sent them again and
the buffer grew
+ without bound while the database failure persisted.
Review Comment:
One sentence is enough here. Use "when committing a transaction fails" if
the retry on connect stays.
```suggestion
Clear the buffer of a database appender when connecting or
committing fails.
```
--
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]
