[cpp-client] Fix for not respecting custom LoggerFactory client config

[smazurov]

Motivation

I want to use custom logging factory in cpp client

Modifications

#7132 broke custom logger config option. It then was identified in #7503 (and #7502) but not fixed. This PR actually fixes it by checking the right variable name.

Verifying this change

• Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Documentation

This PR does not introduce any new functionality, but it fixes existing functionality that presumably is already documented.

[BewareMyPower]

/pulsarbot run-failure-checks

[BewareMyPower]

/pulsarbot run-failure-checks

[BewareMyPower]

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 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.

[smazurov]

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.

[BewareMyPower]

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:

static std::atomic<LoggerFactory*> s_loggerFactory(nullptr);  // setLoggerFactory changes it

And each compile unit includes LogUtils.h to use a thread local logger:

#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;                                                                              \
    }

[smazurov]

Ah yes, of course. We can always change it back after test assets pass

[BewareMyPower]

/pulsarbot run-failure-checks

[BewareMyPower]

/pulsarbot run-failure-checks

[BewareMyPower]

/pulsarbot run-failure-checks

[smazurov]

what is preventing this from getting merged?

[BewareMyPower]

@merlimat @jiazhai @sijie PTAL

[jiazhai]

Thanks @smazurov for the help.