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 2021/04/07 15:34:58 UTC

[GitHub] [pulsar] astifter opened a new pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

astifter opened a new pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164


   Fixes #10161.
   
   ### Motivation
   
   With issue #7132 the LoggerFactoryPtr was removed in favour of relevant memory-safe(r) pointers.
   
   This change forgot to also change the Log4Cxx implementation, fix this by returning (as with
   the other LoggerFactory's) a std::unique_ptr<LoggerFactory>.
   
   ### Modifications
   
   Change remaining LoggerFactoryPtr in Log4Cxx factory to std::unique_ptr<LoggerFactory> to conform to #7132.
   
   ### Verifying this change
   
     - Visual inspection of changes to comply with the ones from #7132.
     - pulsar-client-cpp builds now with USE_LOG4CXX.


-- 
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] astifter commented on pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
astifter commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-815047569


   @merlimat You wan't to base this on master?


-- 
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] astifter commented on pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
astifter commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-845877376


   > @astifter thanks for your code work. For this PR, do we need to update docs?
   
   No, this just fixes the compile.
   
   


-- 
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 #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-845883677


   LGTM. This bug was not exposed because CI only build cpp client with `USE_LOG4CXX=OFF`.


-- 
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] codelipenghui merged pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164


   


-- 
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] astifter commented on pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
astifter commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-845927448


   > I misunderstood, the name of the branch on your repo is "branch-2.7" but the target branch is "master"
   
   Yes, sorry about that. I was working of the 2.7 branch and didn't pay attention. I couldn't change the branch easily so I kept the name. Promise to be more careful in the future.
   
   


-- 
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] codelipenghui closed pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
codelipenghui closed pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164


   


-- 
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] eolivelli commented on pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-845926069


   I misunderstood, the name of the branch on your repo is "branch-2.7" but the target branch is "master"


-- 
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] astifter commented on pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
astifter commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-829525822


   Sorry, missed the deadline for 2.7.2, but I have rebased the change on master.


-- 
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] merlimat commented on pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-821692968


   @astifter Yes, the PR should be targeting master. Once it's merged then it will be cherry picked for on the 2.7 branch


-- 
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] eolivelli commented on pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-827388710


   @astifter can you send a PR for master branch ?
   I am going to cut 2.7.2 release as soon as possible, it would be a pity to not include this patch.


-- 
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] Anonymitaet commented on pull request #10164: [Issue 10161] Fix missing LoggerFactoryPtr type.

Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #10164:
URL: https://github.com/apache/pulsar/pull/10164#issuecomment-844906108


   @astifter thanks for your code work. For this PR, do we need to update docs?


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