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

At Least Once Delivery on Graceful Shutdown #367

Merged
merged 14 commits into from Dec 18, 2017
Merged

At Least Once Delivery on Graceful Shutdown #367

merged 14 commits into from Dec 18, 2017

Conversation

pulltab
Copy link
Contributor

@pulltab pulltab commented Dec 13, 2017

The following implements at-least-once delivery semantics on graceful shutdown.

We achieve this by:

  • Requiring sources / filters to inject Shutdown events into downstream hopper queues when they can ensure that no more events will be placed.

  • Restricting source/filter shutdown such that each incident producer has emitted a Shutdown. This ensures that all possible events have been consumed from each producer.

pulltab added 2 commits Dec 13, 2017
Towards at-least-once delivery - sources now propagate
shutdown to all downstream consumers.  This ensures that Shutdown
events flow in a controlled way from roots of the routing topology
through to leaf nodes.

Sinks and filters shutdown immediately upon receipt of a shutdown.
Thus, we have no guarentee all events are processed prior to shutdown in
cases when multiple sources feed into the same sinks and/or filters.
The final step towards at least once delivery - sinks and filters now shutdown
after their incident producers each have sent a Shutdown event.  This
ensures that, in cases when multiple producers are inputing into a
sink/filter, that each has finished emitting other metric types.
Thus, no more events are expected and it is safe to shutdown.

To accomplish this we had to inform sinks/filters of the sources/filters
which generate data into their receiver. Facilitating this, we now build up
an adjacency matrix and use it to both populate senders for
sources/filters and producer metadata for filters/sinks.
@pulltab pulltab changed the title WIP - Sources Now Shutdown Sinks, Filters At Least Once Delivery on Shutdown Dec 15, 2017
@pulltab pulltab requested review from blt, dparton and gliush Dec 15, 2017
@pulltab pulltab changed the title At Least Once Delivery on Shutdown At Least Once Delivery on Graceful Shutdown Dec 15, 2017
@@ -62,15 +63,25 @@ pub trait Filter {
fn run(
&mut self,
recv: hopper::Receiver<metric::Event>,
sources: std::vec::Vec<std::string::String>,
Copy link
Collaborator

@blt blt Dec 15, 2017

Choose a reason for hiding this comment

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

I do believe you can make this just Vec<String>, no? There are some things auto-used.

Copy link
Contributor Author

@pulltab pulltab Dec 15, 2017

Choose a reason for hiding this comment

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

Shortened everywhere.

Along with String and Option.

src/matrix.rs Outdated

/// Adds an outbound edge from a node to another.
pub fn add_asymmetric_edge(&mut self, from_str: &str, to_str: &str, metadata: std::option::Option<M>) {
let to = std::string::String::from_str(to_str).unwrap();
Copy link
Collaborator

@blt blt Dec 15, 2017

Choose a reason for hiding this comment

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

I think this can be trimmed down to String::from_str.

Copy link
Contributor Author

@pulltab pulltab Dec 15, 2017

Choose a reason for hiding this comment

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

Fixed, and yes it can!

@blt
Copy link
Collaborator

blt commented Dec 15, 2017

@pulltab suggest running clippy on this. Latest nightly / clippy build. There's some clones of double references and what not in the new code that it points out.

}
if let Some(ref configs) = args.statsds {
for (config_path, config) in configs {
config_topology.insert(config_path.clone(), config.forwards.clone());
config_topology.insert(config_path.clone(), config.forwards.clone());
Copy link
Contributor

@dparton dparton Dec 15, 2017

Choose a reason for hiding this comment

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

Nitpick: indent introduced

Copy link
Contributor Author

@pulltab pulltab Dec 15, 2017

Choose a reason for hiding this comment

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

Fixed.

}
}
if let Some(ref configs) = args.graphites {
for (config_path, config) in configs {
config_topology.insert(config_path.clone(), config.forwards.clone());
config_topology.insert(config_path.clone(), config.forwards.clone());
Copy link
Contributor

@dparton dparton Dec 15, 2017

Choose a reason for hiding this comment

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

Nitpick: indent introduced

Copy link
Contributor Author

@pulltab pulltab Dec 15, 2017

Choose a reason for hiding this comment

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

Will fix with a rustfmt run.

Copy link
Contributor Author

@pulltab pulltab Dec 15, 2017

Choose a reason for hiding this comment

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

Fixed.

}
}
if let Some(ref configs) = args.files {
for config in configs {
let config_path = cfg_conf!(config);
config_topology.insert(config_path.clone(), config.forwards.clone());
config_topology.insert(config_path.clone(), config.forwards.clone());
Copy link
Contributor

@dparton dparton Dec 15, 2017

Choose a reason for hiding this comment

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

Nitpick: indent introduced

Copy link
Contributor Author

@pulltab pulltab Dec 15, 2017

Choose a reason for hiding this comment

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

Will fix with a rustfmt run.

Copy link
Contributor Author

@pulltab pulltab Dec 15, 2017

Choose a reason for hiding this comment

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

Fixed.

Some(&mut flush_sends),
&config.forwards,
&cfg_conf!(config),
&config_path,
//&cfg_conf!(config),
Copy link
Contributor

@dparton dparton Dec 15, 2017

Choose a reason for hiding this comment

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

Nitpick: comment left behind

Copy link
Contributor Author

@pulltab pulltab Dec 15, 2017

Choose a reason for hiding this comment

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

Fixed

pulltab added 7 commits Dec 15, 2017
Change moves population of filter sources after forwards have been
populated.  This allows filters to distinguish between incident edges by
the lack of a sender.
Previous logic didn't distinguish between 0 system events and >0 system
events.  The net result was file servers shutting down after a single
iteration.
source_worker
.readiness
.set_readiness(mio::Ready::readable())
.expect("Oops!");
.expect("Failed to set readiness!");
Copy link
Collaborator

@blt blt Dec 18, 2017

Choose a reason for hiding this comment

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

Heh, that's a nicer error message than what I had.

// from each of its
// upstream sources/filters.
total_shutdowns += 1;
if total_shutdowns >= sources.len() {
Copy link
Collaborator

@blt blt Dec 18, 2017

Choose a reason for hiding this comment

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

So cool.

fn run(
&mut self,
recv: hopper::Receiver<metric::Event>,
sources: std::vec::Vec<String>,
Copy link
Collaborator

@blt blt Dec 18, 2017

Choose a reason for hiding this comment

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

This could be shortened to Vec.

blt
blt previously approved these changes Dec 18, 2017
Copy link
Collaborator

@blt blt left a comment

Boy I'm excited about this.

blt
blt approved these changes Dec 18, 2017
@pulltab pulltab merged commit a4da52d into master Dec 18, 2017
2 checks passed
@pulltab pulltab deleted the at-least-once branch Dec 18, 2017
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

3 participants