Skip to content

Conversation

jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Aug 7, 2018

Description: envoy/stats/stats.h was a collection of many loosely related class interfaces. This PR breaks them up, mostly one class per file, except for a few closely related ones. One benefit of this is that code changes can be made much more quickly, without recompiling the world on every edit. E.g. many uses of Stats only need Scope or StatsOptions, and don't need many of the other classes that change more frequently. Another benefit is simply to offer a better parallel to the implementation breakup already merged.

Risk Level: low
Testing: //test/...
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz requested review from lizan and zuercher as code owners August 7, 2018 01:16
@@ -10,7 +10,19 @@ envoy_package()

envoy_cc_library(
name = "stats_interface",
hdrs = ["stats.h"],
hdrs = [
Copy link
Member

Choose a reason for hiding this comment

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

can BUILD targets be split to multiple ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I was thinking of doing that in a follow-up as that will be another large PR.

…uild rules.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Epic!

@mattklein123 mattklein123 merged commit c68063c into envoyproxy:master Aug 7, 2018
@jmarantz jmarantz deleted the stats-impl-atomization branch August 7, 2018 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants