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

Add Census base log #5098

Merged
merged 9 commits into from
Feb 16, 2016
Merged

Add Census base log #5098

merged 9 commits into from
Feb 16, 2016

Conversation

a-veitch
Copy link
Contributor

@a-veitch a-veitch commented Feb 5, 2016

This currently depends on #5096 for passing the test on windows. Don't merge until that is.

Fixes #4666

@a-veitch a-veitch added this to the Stats and Tracing milestone Feb 5, 2016
@grpc-kokoro
Copy link

Can one of the admins verify this patch?

1 similar comment
@grpc-kokoro
Copy link

Can one of the admins verify this patch?

@a-veitch
Copy link
Contributor Author

a-veitch commented Feb 8, 2016

#5096 has been merged, and I've merged this branch to head. Hopefully all tests should now pass.

bogdandrutu
bogdandrutu reviewed Feb 9, 2016
View reviewed changes
src/core/census/log.c
gpr_atm block;
/* Ensure cachline alignment: we assume sizeof(gpr_atm) == 4 or 8 */
#if defined(GPR_ARCH_64)
#define CL_CORE_LOCAL_BLOCK_PAD_SIZE (GPR_CACHELINE_SIZE - 8)
Copy link

Choose a reason for hiding this comment

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

Why not sizeof(gpr_atm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because (very annoyingly) sizeof gets evaluated by the compiler, not the preprocessor, which means the #if on line 169 would fail to evaluate.

bogdandrutu
bogdandrutu reviewed Feb 11, 2016
View reviewed changes
test/core/census/log_test.c
memset(record, data, size);
}

/* Reads fixed size records. Returns the number of records read in
Copy link

Choose a reason for hiding this comment

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

Sorry for causing more work :) Consistency is for the win . Please update these comments as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, comments updated.

@bogdandrutu
Copy link

Merge attempt failed
Head branch was modified. Review and try the merge again.

bogdandrutu pushed a commit that referenced this pull request Feb 16, 2016
Merge pull request #5098 from a-veitch/base_log
43f6311 
Add Census base log
@bogdandrutu bogdandrutu merged commit 43f6311 into grpc:master Feb 16, 2016
@a-veitch a-veitch deleted the base_log branch February 18, 2016 22:56
@makdharma
Copy link
Contributor

ObjC travis build is broken with this problem:

Undefined symbols for architecture i386:
"_gpr_log_message", referenced from:
_gpr_log in libgRPC.a(log_posix.o)
"_gpr_log_severity_string", referenced from:
_gpr_default_log in libgRPC.a(log_posix.o)
ld: symbol(s) not found for architecture i386
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@a-veitch
Copy link
Contributor Author

Not sure why you're commenting on ObjC breakage on this PR?

@jcanizales
Copy link
Contributor

Because it was broken by this PR, and detected by Travis, which was ignored along with the other failed automated tests :)

@a-veitch
Copy link
Contributor Author

Aargh. I was sure those tests passed sometime. OK, now I have to figure out how/why this change, triggers this problem in the ObjC build. Looking at this error message:

Undefined symbols for architecture i386:
"_gpr_log_message", referenced from:
_gpr_log in libgRPC.a(log_posix.o)

implies that for some reason src/core/support/log.c isn't being linked?

And the lightbulb goes off - same filename, different directories. But why only ObjC?

Tomorrow.

@a-veitch
Copy link
Contributor Author

I've confirmed that this issue is due to the duplicate file names, even though the two log.o files are in separate libraries. A quick hack would be to use a different name for one of them. A better solution would be to do "the right thing" - trying to see if there is a libtool option to do that.

@jcanizales
Copy link
Contributor

Tools that assume nobody would ever put code in a subdirectory are the bane of Mac development.

@a-veitch
Copy link
Contributor Author

OK, I have a fix. Turns out the problem was really that we were linking the libgpr objects into libgrpc.a. Removing that seems to have everything working. Let me do some more tests, and will send you a PR.

@a-veitch a-veitch mentioned this pull request Feb 20, 2016
Closed
@makdharma
Copy link
Contributor

Hi, the master is still failing. Have you submitted the PR already?

Thanks

On Fri, Feb 19, 2016 at 5:07 PM, Alistair Veitch notifications@github.com
wrote:

OK, I have a fix. Turns out the problem was really that we were linking
the libgpr objects into libgrpc.a. Removing that seems to have everything
working. Let me do some more tests, and will send you a PR.

—
Reply to this email directly or view it on GitHub
#5098 (comment).

@a-veitch
Copy link
Contributor Author

#5352 is out for review.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@bogdandrutu bogdandrutu

Labels
area/Census cla: yes lang/core
Projects
None yet
Milestone
Stats and Tracing
Development

Successfully merging this pull request may close these issues.
6 participants
@a-veitch @grpc-kokoro @bogdandrutu @makdharma @jcanizales @googlebot