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.

Sign up for GitHub

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

Jump to bottom

[Issue 10161] Fix missing LoggerFactoryPtr type. #10164

Merged
merged 1 commit into from
May 21, 2021
Merged

[Issue 10161] Fix missing LoggerFactoryPtr type. #10164

merged 1 commit into from
May 21, 2021
+7 −6

Conversation

astifter
Copy link
Contributor

@astifter astifter commented Apr 7, 2021

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.

Modifications

Change remaining LoggerFactoryPtr in Log4Cxx factory to std::unique_ptr to conform to #7132.

Verifying this change

@merlimat merlimat added this to the 2.8.0 milestone Apr 7, 2021
@astifter
Copy link
Contributor Author

astifter commented Apr 7, 2021

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

@merlimat
Copy link
Contributor

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

@eolivelli
Copy link
Contributor

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

eolivelli
eolivelli requested changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

I am commenting with "Request changes" in order to clarify the status of this patch

@astifter
Fix issue apache#10161.
ba847ea 
With issue apache#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>.
@astifter astifter changed the base branch from branch-2.7 to master April 29, 2021 19:24
@astifter
Copy link
Contributor Author

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

@Anonymitaet
Copy link
Member

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

@codelipenghui codelipenghui requested a review from eolivelli May 21, 2021 10:07
@codelipenghui codelipenghui reopened this May 21, 2021
@astifter
Copy link
Contributor Author

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

No, this just fixes the compile.

BewareMyPower
BewareMyPower approved these changes May 21, 2021
View reviewed changes
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

LGTM

@BewareMyPower
Copy link
Contributor

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

@eolivelli
Copy link
Contributor

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

@codelipenghui codelipenghui merged commit d700f8d into apache:master May 21, 2021
@astifter
Copy link
Contributor Author

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.

yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
@astifter @yangl
[Issue 10161] Fix missing LoggerFactoryPtr type. (apache#10164)
759b221 
Fixes apache#10161.

### Motivation

With issue apache#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>.
codelipenghui pushed a commit that referenced this pull request Jun 26, 2021
@astifter @codelipenghui
[Issue 10161] Fix missing LoggerFactoryPtr type. (#10164)
b272df4 
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>.

(cherry picked from commit d700f8d)
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jun 26, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@astifter @ciaocloud
[Issue 10161] Fix missing LoggerFactoryPtr type. (apache#10164)
376192e 
Fixes apache#10161.

### Motivation

With issue apache#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>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat

@sijie sijie

@eolivelli eolivelli

@BewareMyPower BewareMyPower

Assignees

@astifter astifter

Labels
area/build cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.3
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

Pulsar 2.7.1 doesn't build with USE_LOG4CXX=ON
7 participants
@astifter @merlimat @eolivelli @Anonymitaet @BewareMyPower @sijie @codelipenghui