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.

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

Avoid using a map for log attributes #34174

Merged
merged 1 commit into from Jul 21, 2017
Merged

Conversation

@aaronlehmann
Copy link
Contributor

aaronlehmann commented Jul 19, 2017

Having a map per log entry seemed heavier than necessary. These attributes end up being sorted and serialized, so storing them in a map doesn't add anything (there's no random access element). In SwarmKit, they originate as a slice, so there's an unnecessary conversion to a map and back.

This also fixes the sort comparator, which used to inefficiently split the string on each comparison.

Fixes #32418

cc @dperny

Having a map per log entry seemed heavier than necessary. These
attributes end up being sorted and serialized, so storing them in a map
doesn't add anything (there's no random access element). In SwarmKit,
they originate as a slice, so there's an unnecessary conversion to a map
and back.

This also fixes the sort comparator, which used to inefficiently split
the string on each comparison.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 19, 2017

This is only an internal change, no change to the REST API?

@aaronlehmann

This comment has been minimized.

Copy link
Contributor Author

aaronlehmann commented Jul 19, 2017

This is only an internal change, no change to the REST API?

That's right.

Copy link
Contributor

cpuguy83 left a comment

LGTM

@dperny

This comment has been minimized.

Copy link
Contributor

dperny commented Jul 20, 2017

LGTM, tho I am unsure about the possible effects on third-party logging drivers.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 20, 2017

ping @cpuguy83 any concerns about that? ^^

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jul 21, 2017

No problem for plugins.

Copy link
Member

thaJeztah left a comment

thanks! LGTM

@thaJeztah thaJeztah merged commit 901fe35 into moby:master Jul 21, 2017
6 checks passed
6 checks passed
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 35752 has succeeded
Details
janky Jenkins build Docker-PRs 44352 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 4730 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 15731 has succeeded
Details
z Jenkins build Docker-PRs-s390x 4429 has succeeded
Details
@cpuguy83 cpuguy83 mentioned this pull request Aug 31, 2017
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.