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

Test and clean #728

Merged
merged 5 commits into from Nov 21, 2014
Merged

Test and clean #728

merged 5 commits into from Nov 21, 2014

Conversation

mbethke
Copy link

@mbethke mbethke commented Sep 10, 2014

I use collectd on CentOS which has Regexp::Common available only via the EPEL repo, deprecated at $WORKPLACE due to its rather mixed code quality. Thus I replaced Regexp::Common by the core module Scalar::Util, and while I was at it improved a few more things:

  • Added a small test suite
  • Cleaned up some of the Perl style where it was very C-ish, resulting in quite a bit shorter code
  • Refactored some common code bits into private methods
  • Added a new method listval_filter that is over five times faster (for a common task such as finding all disks for any single host in my particular application - currently 19 hosts with just under 5k metrics; rather more for larger installations) than listval()

- Remove some superfluous parenthesis clutter
- Shorten a lot of single-line conditionals using postfix constructions
- Merge variable declarations
- Use $class/$self instead of $pkg/$obj as is customary
- Remove quotes around literal hash keys
Rationale:
A frequent use case for LISTVAL is to retrieve a list of resources for a certain
host or host group that is not known in advance, such as when the hosts have
different disks installed. Using the existing listval() method,
Collectd::Unixsock retrieves the entire list, parses each entry into a hash and
returns the list, only to have the caller throw away the vast majority of
entries immediately. listval_filter() allows the caller to pass any attribute
that can be passed to getval() and filters the list of resources retrieved from
the socket before parsing it, resulting in a large speedup.
The current implemntation has some code duplication, although listval() could be
implemented as a small wrapper around listval_filter() with just a few percent
speed penalty due to the extra dynamically built regexp.
@faxm0dem
Copy link
Contributor

This certainly looks like fine work to me, thanks!
I'll test it ASAP

@pyr
Copy link
Member

pyr commented Nov 21, 2014

Thanks a lot @mbethke !

pyr added a commit that referenced this pull request Nov 21, 2014
@pyr pyr merged commit 04b620a into collectd:master Nov 21, 2014
@mbethke mbethke deleted the test-and-clean branch December 3, 2014 18:06
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.

None yet

4 participants