Skip to content

stats: create StatNameSet via SymbolTable::makeSet rather than construction.#8369

Merged
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
jmarantz:stat-name-set-ownership
Sep 26, 2019
Merged

stats: create StatNameSet via SymbolTable::makeSet rather than construction.#8369
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
jmarantz:stat-name-set-ownership

Conversation

@jmarantz
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz commented Sep 25, 2019

Description: Split from #8116 -- refactor the StatNameSet construction so it must be done via a method on SymbolTable. This allows, in #8116, recent-lookup requests to be aggregated across the SymbolTable and all associated sets.

This also adds a name for the sets, though it's not used for anything yet.
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>
…ing them.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz changed the title WiP: stats: create StatNameSet via SymbolTable::makeSet rather than construction. stats: create StatNameSet via SymbolTable::makeSet rather than construction. Sep 25, 2019
@jmarantz
Copy link
Copy Markdown
Contributor Author

Note the CI failures are due to out-of-disk-space in azp.

Copy link
Copy Markdown
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.

Thanks looks great with a few small comments.

/wait

// This must be held during both encode() and free().
mutable Thread::MutexBasicLockable lock_;

// This must be held while updating stat_name_sets_.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For simplicity thoughts on just using the lock you already have defined? Do we really need 2 locks?


StatNameSet(SymbolTable& symbol_table, absl::string_view name);

std::string name_;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: const

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Copy link
Copy Markdown
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.

Cool, thanks.

@mattklein123 mattklein123 merged commit 3068ba9 into envoyproxy:master Sep 26, 2019
@jmarantz jmarantz deleted the stat-name-set-ownership branch September 26, 2019 23:09
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
…uction. (envoyproxy#8369)

Signed-off-by: Joshua Marantz <jmarantz@google.com>
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.

2 participants