periodically check connectivity to Cloud Pubsub #1031

mattnworb · 2016-12-02T20:47:11Z

This is an alternate implementation of #1030 which performs the healthchecks periodically rather than only once at process startup.

mattnworb · 2016-12-02T20:50:21Z

changes compared to #1030:

GooglePubSubSender checks a healthchecker interface each time that send(topic, message) is called.
- the healthchecking logic is broken out into interface/implementation to make testing of the EventSender logic and the healthcheck logic simpler
- the default healthchecker implementation does a check every 5 minutes to see if pubsub.getTopic(..) is successful or not.
EventSenderFactory registers all of the EventSenders and the PubSub client instance with the dropwizard environment lifecycle, so that everything can be cleanly shutdown when the service shuts down.
the EventSender interface now extends Managed from dropwizard, so that we can shut things down - I wanted to be able to shutdown the executor used for the Cloud Pubsub healthchecks, and along the way I noticed that the KafkaProvider was never cleanly shut down
eliminated GooglePubSubProvider as it wasn't really doing much

the CommonConfiguration stuff is unchanged from #1030.

mattnworb · 2016-12-02T20:51:52Z

@davidxia @lndbrg @rohansingh

codecov-io · 2016-12-02T21:17:58Z

Current coverage is 51.14% (diff: 35.29%)

Merging #1031 into master will increase coverage by 4.76%

@@             master      #1031   diff @@
==========================================
  Files           107        275    +168   
  Lines          5095      13157   +8062   
  Methods           0          0           
  Messages          0          0           
  Branches        858       1699    +841   
==========================================
+ Hits           2363       6729   +4366   
- Misses         2514       5925   +3411   
- Partials        218        503    +285

Powered by Codecov. Last update 0367d6f...fbe6378

davidxia

LGTM but would like another pair of eyes.

davidxia · 2016-12-06T17:37:17Z

helios-services/src/main/java/com/spotify/helios/servicescommon/GooglePubSubSender.java

+    this.healthchecker = healthchecker;
+  }
+
+//  public void start() {


Delete this commented out stuff?

it is removed in a later commit

davidxia · 2016-12-06T17:43:33Z

helios-services/src/main/java/com/spotify/helios/servicescommon/GooglePubSubSender.java

+    boolean isHealthy();
+  }
+
+  public static class DefaultHealthChecker implements HealthChecker {


nitpick: This and the constructor be package-local.

davidxia · 2016-12-06T17:48:32Z

helios-services/src/main/java/com/spotify/helios/servicescommon/KafkaSender.java

-    }
+  @Override
+  public void stop() throws Exception {
+    kafkaProducer.close();


Good catch.

davidxia · 2016-12-06T17:51:16Z

helios-services/src/main/java/com/spotify/helios/servicescommon/EventSenderFactory.java

+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+
+public final class EventSenderFactory {


Is it worth testing this class or too much of a pain?

i think it would be too cumbersome as this is constructing new instances of KafkaSender GooglePubSubSender etc directly

lndbrg · 2016-12-07T08:47:27Z

helios-services/src/main/java/com/spotify/helios/servicescommon/GooglePubSubSender.java

  }

  @Override
  public void send(final String topic, final byte[] message) {
    final String combinedTopic = topicPrefix + topic;
+
+    if (!healthchecker.isHealthy()) {
+      log.warn("will not publish message to pubsub topic={} as the pubsub client "


You're missing the topic for interpolation here.

good catch, thanks

please smash that approve button now

when the GooglePubSubSender cannot connect to the pubsub service, disable message publishing until connectivity is re-established (at the next scheduled time). Along the way, I refactored the common EventSender-instantiation code from AgentService and MasterService into a new class named EventSenderFactory. To make this common class possible, also extracted a class for the common configuration fields between AgentConfig and MasterConfig. This new CommonConfiguration class is incomplete - more fields can be moved to this class to avoid duplication, but I left that for future commits.

make all the EventSender implementations also implement the dropwizard Managed interface so that we can have dropwizard manage their lifecycle. This lets us cleanly shutdown the KafkaProvider or the executor used by GooglePubSubSender.

Add a Gauge for whether or not the GooglePubSubSender is healthy. Refactor the builder for GooglePubSubSender to move the creation of the healthchecker, ScheduledExecutorService etc to EventSenderFactory.

this class is only ever constructed with a non-empty Optional, so we can simplify it by removing the Optional.

mattnworb force-pushed the pubsub-healthcheck-periodically branch from 956df2b to 47d8b35 Compare December 2, 2016 20:52

mattnworb force-pushed the pubsub-healthcheck-periodically branch 2 times, most recently from 6f14735 to 0e327c1 Compare December 2, 2016 21:47

davidxia reviewed Dec 6, 2016

View reviewed changes

lndbrg suggested changes Dec 7, 2016

View reviewed changes

mattnworb added 4 commits December 7, 2016 10:14

start/stop EventSenders when service starts/stops

cee0cf4

make all the EventSender implementations also implement the dropwizard Managed interface so that we can have dropwizard manage their lifecycle. This lets us cleanly shutdown the KafkaProvider or the executor used by GooglePubSubSender.

add metric gauges for pubsub health

f704368

Add a Gauge for whether or not the GooglePubSubSender is healthy. Refactor the builder for GooglePubSubSender to move the creation of the healthchecker, ScheduledExecutorService etc to EventSenderFactory.

simplify KafkaSender by removing Optional wrapper

fbe6378

this class is only ever constructed with a non-empty Optional, so we can simplify it by removing the Optional.

mattnworb force-pushed the pubsub-healthcheck-periodically branch from 0e327c1 to fbe6378 Compare December 7, 2016 15:14

davidxia approved these changes Dec 7, 2016

View reviewed changes

davidxia merged commit 9cb7082 into master Dec 9, 2016

davidxia deleted the pubsub-healthcheck-periodically branch December 9, 2016 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

periodically check connectivity to Cloud Pubsub #1031

periodically check connectivity to Cloud Pubsub #1031

mattnworb commented Dec 2, 2016

mattnworb commented Dec 2, 2016 •

edited

Loading

mattnworb commented Dec 2, 2016

codecov-io commented Dec 2, 2016 •

edited

Loading

davidxia left a comment

davidxia Dec 6, 2016

davidxia Dec 7, 2016

mattnworb Dec 7, 2016

davidxia Dec 6, 2016

davidxia Dec 6, 2016

davidxia Dec 6, 2016

mattnworb Dec 7, 2016

lndbrg Dec 7, 2016

mattnworb Dec 7, 2016

mattnworb Dec 7, 2016

mattnworb Dec 7, 2016

periodically check connectivity to Cloud Pubsub #1031

periodically check connectivity to Cloud Pubsub #1031

Conversation

mattnworb commented Dec 2, 2016

mattnworb commented Dec 2, 2016 • edited Loading

mattnworb commented Dec 2, 2016

codecov-io commented Dec 2, 2016 • edited Loading

Current coverage is 51.14% (diff: 35.29%)

davidxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattnworb commented Dec 2, 2016 •

edited

Loading

codecov-io commented Dec 2, 2016 •

edited

Loading