You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/08/28 20:22:57 UTC
[GitHub] [pulsar] smazurov opened a new pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
smazurov opened a new pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932
### Motivation
I want to use custom logging factory in cpp cient
### Modifications
https://github.com/apache/pulsar/pull/7132 broke custom logger config option. It then was identified in https://github.com/apache/pulsar/pull/7503 but not fixed. This PR actually fixes it.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
No.
### Documentation
This PR does not introduce any new functionality, but it fixes existing functionality that presumably is already documented.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-684841140
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-689898061
Thanks @smazurov for the help.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-687580663
@merlimat @jiazhai @sijie PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-683535277
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-684787561
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] smazurov commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
smazurov commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-683596331
Ah yes, of course. We can always change it back after test assets pass
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-684892716
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-683580112
You're right, the internal field of logger should be not a `fstream` but a `vector<string>`. Here I use the file because if other tests were affected by the custom logger, I could check the failure logs with a file.
The reason why the custom logger affects other tests is that the `ClientConfiguration`'s logger factory is not associated with a `Client`, it only changes a `static` factory, see `LogUtils.cc`:
```c++
static std::atomic<LoggerFactory*> s_loggerFactory(nullptr); // setLoggerFactory changes it
```
And each compile unit includes `LogUtils.h` to use a thread local logger:
```c++
#define DECLARE_LOG_OBJECT() \
static pulsar::Logger* logger() { \
static thread_local std::unique_ptr<pulsar::Logger> threadSpecificLogPtr; \
pulsar::Logger* ptr = threadSpecificLogPtr.get(); \
if (PULSAR_UNLIKELY(!ptr)) { \
std::string logger = pulsar::LogUtils::getLoggerName(__FILE__); \
threadSpecificLogPtr.reset(pulsar::LogUtils::getLoggerFactory()->getLogger(logger)); \
ptr = threadSpecificLogPtr.get(); \
} \
return ptr; \
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] smazurov commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
smazurov commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-687330830
what is preventing this from getting merged?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] smazurov commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
smazurov commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-683558582
I think a better test would be to log internally and just check that, that way you're not dealing with files, etc. I'm not sure why it would affect other tests though, that seems odd.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-683538925
LGTM. Since the bug was introduced because there's no way to verify for custom logger currently, I thought it could be better to add an unit test for custom logger, e.g. use a custom logger to write logs to a file with a specific prefix, then read the file to check if every line has the prefix.
I have written a [simple test](https://gist.github.com/BewareMyPower/c304fb33a77f408f1247143958088030) and run the tests with gtest-parallel, it works well. However, without gtest-parallel, logs of other unit tests could be written to the same file because the logger of each file was `thread_local`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932#issuecomment-683525207
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai merged pull request #7932: [cpp-client] Fix for not respecting custom LoggerFactory client config
Posted by GitBox <gi...@apache.org>.
jiazhai merged pull request #7932:
URL: https://github.com/apache/pulsar/pull/7932
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org