Skip to content

implement logger facility and support its' overriding; #19

Merged
merged 16 commits into from
Feb 17, 2022

Conversation

phobosxy
Copy link
Contributor

No description provided.

Copy link
Contributor

@i-vovk i-vovk left a comment

Choose a reason for hiding this comment

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

This is not a robust solution. If we want to log error anywhere else this code will be copied. Also, this approach adds a new constructor which is not great either.
This should be done via registering the logger instance once and using it anywhere we want to log something. You should define an appropriate logger interface, define a dummy one (dumps to stdout), and provide free function to register custom (something like stream_client::set_logger(...)). This function may accept a logger instance or just a callback function and construct an instance from it. Also, the logger interface should support log levels.

@i-vovk
Copy link
Contributor

i-vovk commented Feb 11, 2022

Another approach would be to propagate such errors outside and give the user ability to process them (they may log, count or take action upon them).

@phobosxy phobosxy requested a review from i-vovk February 15, 2022 10:23
@phobosxy phobosxy closed this Feb 15, 2022
@phobosxy phobosxy reopened this Feb 15, 2022
@phobosxy
Copy link
Contributor Author

update by comments in new patchset

include/stream-client/detail/logger.ipp Outdated Show resolved Hide resolved
include/stream-client/logger.hpp Outdated Show resolved Hide resolved
include/stream-client/logger.hpp Outdated Show resolved Hide resolved
include/stream-client/logger.hpp Outdated Show resolved Hide resolved
include/stream-client/detail/logger.ipp Outdated Show resolved Hide resolved
include/stream-client/connector/impl/connection_pool.ipp Outdated Show resolved Hide resolved
include/stream-client/connector/impl/connection_pool.ipp Outdated Show resolved Hide resolved
include/stream-client/detail/logger.ipp Outdated Show resolved Hide resolved
include/stream-client/detail/logger.ipp Outdated Show resolved Hide resolved
include/stream-client/detail/logger.ipp Outdated Show resolved Hide resolved
@i-vovk

This comment was marked as resolved.

include/stream-client/connector/impl/connection_pool.ipp Outdated Show resolved Hide resolved
include/stream-client/impl/logger.ipp Outdated Show resolved Hide resolved
tests/pool.cpp Outdated Show resolved Hide resolved
tests/logger.cpp Outdated Show resolved Hide resolved
@i-vovk i-vovk changed the title CCT-126: use log callback function for connection events; implement logger facility and support its' overriding; Feb 17, 2022
@i-vovk i-vovk merged commit aaf0968 into Tinkoff:develop Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants