Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant graph execution exception logging #41

Open
wants to merge 1 commit into
base: release/1.5
Choose a base branch
from
Open

Remove redundant graph execution exception logging #41

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .../src/main/kotlin/ru/fix/completable/reactor/runtime/execution/HandleByExecutionBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ class HandleByExecutionBuilder<PayloadType>(
""".trimIndent(),
throwable)

log.error(exc) {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such logging has following drawbacks:

  1. it produces message "kotlin.Unit" since empty message lambda provided
  2. What is the motivation of such logging if we call executionResultFuture.completeExceptionally? By doing so library clients loose ability to decide how to handle occurred exception - it is always logged even if graph future still failed with exception. In practice, with this logging error message is duplicated by this logger and client app's logger.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • There was several cases, when application level logging was incorrectly setup. Presence of logger on completable-reactor level helped resolve issues: it provided detailed stack trace information that app level logging missed.
  • By removing this library level logging you making decision on behalf of the the user of application that he should take care of logging in particular way. We removing option: what if number of such errors is small, and user can simply decide do not overwhelm app logic with logging and benefit of the option that completable-reactor itself will provide reasonable diagnostic information if something happens.
  • There are another project: bank131 that actively uses reactor. And seems like there are no problems there with this logging. Maybe better option would be to provide dedicated logger name and in user project simply disable particular log via logging option. E.g.
    here we can change logger name to ru.fix.completable.reactor.user.exception
    And in application we can add option: log4j ru.fix.completable.reactor.user.exception = OFF

executionResultFuture.completeExceptionally(exc)

pvx.handlingFuture.complete(HandlePayloadContext(isTerminal = true))
Expand Down