chibenwa commented on code in PR #3237:
URL: https://github.com/apache/james-project/pull/3237#discussion_r4192162917
##########
backends-common/postgres/src/main/java/org/apache/james/backends/postgres/PostgresTableManager.java:
##########
@@ -96,6 +98,8 @@ public Mono<Void> initializeTables() {
.filter(table ->
!existTables.contains(table.getName()))
.flatMap(table -> createAndAlterTable(table, dsl,
connection))))
.then(),
+ connection ->
postgresExecutor.connectionFactory().closeConnection(connection),
+ (connection, error) ->
postgresExecutor.connectionFactory().closeConnection(connection),
Review Comment:
Likely log the error as this is a terminal exception, here it is lost
##########
mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresModSeqProvider.java:
##########
@@ -87,6 +88,6 @@ public ModSeq highestModSeq(MailboxId mailboxId) {
@Override
public Mono<ModSeq> nextModSeqReactive(MailboxId mailboxId) {
return mailboxDAO.incrementAndGetModSeq(mailboxId)
- .defaultIfEmpty(ModSeq.first());
+ .switchIfEmpty(Mono.error(new
MailboxNotFoundException(mailboxId)));
Review Comment:
Idem
##########
mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/mail/PostgresMessageMapper.java:
##########
@@ -372,7 +373,7 @@ public MessageMetaData copy(Mailbox mailbox, MailboxMessage
original) throws Mai
private Mono<Void> setNewUidAndModSeq(MailboxMessage mailboxMessage) {
return
mailboxDAO.incrementAndGetLastUidAndModSeq(mailboxMessage.getMailboxId())
- .defaultIfEmpty(Pair.of(MessageUid.MIN_VALUE, ModSeq.first()))
+ .switchIfEmpty(Mono.error(new
MailboxNotFoundException(mailboxMessage.getMailboxId())))
Review Comment:
As far as I am aware of, we cannot distinguish "new mailbox with not yet a
UID" from "mailbox that do not exist yet"
Taken in isolation, this code path could look like a minor bug. But without
an existing mailbox this get never called.
I propose we actually drop it.
##########
backends-common/postgres/src/main/java/org/apache/james/backends/postgres/PostgresTableManager.java:
##########
@@ -85,6 +85,8 @@ public Mono<Void> initializePostgresExtension() {
.execute())
.flatMap(Result::getRowsUpdated)
.then(),
+ Connection::close,
+ (connection, error) -> connection.close(),
Review Comment:
Likely log the error as this is a terminal exception, here it is lost
##########
backends-common/postgres/src/main/java/org/apache/james/backends/postgres/PostgresTableManager.java:
##########
@@ -195,6 +203,8 @@ public Mono<Void> initializeTableIndexes() {
.filter(index ->
!existIndexes.contains(index.getName()))
.flatMap(index -> createTableIndex(index, dsl))))
.then(),
+ connection ->
postgresExecutor.connectionFactory().closeConnection(connection),
+ (connection, error) ->
postgresExecutor.connectionFactory().closeConnection(connection),
Review Comment:
Likely log the error as this is a terminal exception, here it is lost
##########
backends-common/postgres/src/main/java/org/apache/james/backends/postgres/PostgresTableManager.java:
##########
@@ -184,6 +190,8 @@ public Mono<Void> truncate() {
.doOnSuccess(any -> LOGGER.info("Table {} truncated",
table.getName()))
.doOnError(e -> LOGGER.error("Error while truncating
table {}", table.getName(), e)))
.then()),
+ connection ->
postgresExecutor.connectionFactory().closeConnection(connection),
+ (connection, error) ->
postgresExecutor.connectionFactory().closeConnection(connection),
Review Comment:
Likely log the error as this is a terminal exception, here it is lost
##########
backends-common/postgres/src/main/java/org/apache/james/backends/postgres/PostgresTableManager.java:
##########
@@ -115,6 +119,8 @@ public Mono<List<String>> listExistTables() {
.eq(DSL.currentSchema()))))
.map(r -> r.get(0, String.class))
.collectList(),
+ connection ->
postgresExecutor.connectionFactory().closeConnection(connection),
+ (connection, error) ->
postgresExecutor.connectionFactory().closeConnection(connection),
Review Comment:
Likely log the error as this is a terminal exception, here it is lost
##########
backends-common/postgres/src/main/java/org/apache/james/backends/postgres/utils/PostgresExecutor.java:
##########
@@ -327,6 +327,17 @@ private <T> Mono<T> handleTimeout(Connection connection,
TimeoutException timeou
.then(Mono.error(timeoutException));
}
+ private <T> Mono<T> handleTransactionTimeout(Connection connection,
TimeoutException timeoutException) {
+ LOGGER.error(JOOQ_TIMEOUT_ERROR_LOG, timeoutException);
+ return cancelRunningQuery(connection)
+ .then(Mono.from(connection.rollbackTransaction())
+ .onErrorResume(e -> {
+ LOGGER.warn("Failed to rollback the timed out Postgres
transaction", e);
+ return Mono.empty();
+ }))
+ .then(Mono.error(timeoutException));
+ }
Review Comment:
A coding agent did output to me that r2dbc already does a transaction
cleanup.
Do we have a test reproducing the issue ?
--
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]
