Skip to content

Conversation

@hectcastro
Copy link
Contributor

This changeset adds support for publishing log entries to a Redis channel, which is also supported by Logstash's Redis input.

Beaver configuration files can now supply a redis_data_type key. Valid values for this key are list and channel. If left unset, the default is list.

Attempts to resolve #266.

This changeset adds support for publishing log entries to a Redis
channel, which is also supported by Logstash's Redis input.

Beaver configuration files can now supply a `redis_data_type` key. Valid
values for this key are `list` and `channel`. If left unset, the default
is `list`.

Attempts to resolve #266.
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this check outside of the for loop? That way we aren't doing the check for each line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in https://github.com/hectcastro/python-beaver/commit/94812db5e13a95bf20da26bc616e6f12973deff4. Sorry for the original, I wasn't quite sure how to call a method dynamically in Python.

Copy link
Member

Choose a reason for hiding this comment

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

We should do this checking in the __init__, though the method setting can stay here.

Something like:

callback_map = {
    'list': pipeline.rpush,
    'channel': pipeline.publish,
}

callback_method = callback_map[data_type]

Would be a bit nicer 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.

@josegonzalez
Copy link
Member

My only other concern is that instead of being the "data type", why not just call it "redis_transport_method"? Valid methods would be publish and rpush, which would allow us to use other methods in the future.

@hectcastro
Copy link
Contributor Author

The main driver for going with list and channel via a data type option was to mirror the data_type option of Logstash's Redis input. Since Beaver is meant to interact with Logstash, it made sense to align their options for transport.

@josegonzalez
Copy link
Member

Gotcha, I thought they also supported lpush if configured?

@hectcastro
Copy link
Contributor Author

From what I've seen in the Logstash Redis input plugin code, BLPOP is always used to interact with a list. Supporting LPUSH would put the most recent log message at the beginning of the list, which could work against users if things get congested. It is unclear to me what benefits supporting that method would provide.

@josegonzalez
Copy link
Member

Hmm okay. I'll merge this then.

josegonzalez added a commit that referenced this pull request Jun 11, 2015
@josegonzalez josegonzalez merged commit 5a0a860 into python-beaver:master Jun 11, 2015
@hectcastro hectcastro deleted the hmc/redis-transport-channel branch June 11, 2015 19:18
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.

Redis Channels?

2 participants