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.

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

Conversation

TimurKasatkin
Copy link
Member

No description provided.

@TimurKasatkin TimurKasatkin changed the base branch from master to release/1.5 April 18, 2021 19:07
@@ -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

@@ -190,7 +190,6 @@ class HandleByExecutionBuilder<PayloadType>(
""".trimIndent(),
throwable)

log.error(exc) {}
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants