Reduce and smooth traffic on drive back ends when requesting change l…

…ists.

As requested by Drive SRE these changes reduce the frequency of requests on the
drive back ends, and also lessens the likelihood of convoying.

1. When XMPP is connected, change the poll timeout for changes from 5 minutes to
   between 60 & 120 minutes.
2. When XMPP is disconnected, change the poll timeout for changes from 1 minute
   to between 1 and 2 minutes. This issues start page token requests which are
   cheap for the drive back ends.
3. When batching XMPP notifications, change the batch time from 5 seconds to
   between 15 and 30 seconds. This reduces the QPS on drive back ends when a
   team drive change notification is broadcast to a large corpus of users, and
   prevents convoying of requests by introducing jitter.

Bug: 905201
Change-Id: I5310d956c9ccb5b59261cdf99bbefb515f75771f
Reviewed-on: https://chromium-review.googlesource.com/c/1336949
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Stuart Langley <slangley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608239}

components/drive/drive_notification_manager.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/metrics/histogram_macros.h"
+#include "base/rand_util.h"
 #include "base/strings/char_traits.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
@@ -25,11 +26,10 @@ const int kFastPollingIntervalInSecs = 60;
 
 // The polling interval time is used when XMPP is enabled.  Theoretically
 // polling should be unnecessary if XMPP is enabled, but just in case.
-const int kSlowPollingIntervalInSecs = 300;
+const int kSlowPollingIntervalInSecs = 3600;
 
 // The period to batch together invalidations before passing them to observers.
-constexpr base::TimeDelta kInvalidationBatchInterval =
-    base::TimeDelta::FromSeconds(5);
+constexpr int kInvalidationBatchIntervalSecs = 15;
 
 // The sync invalidation object ID for Google Drive.
 const char kDriveInvalidationObjectId[] = "CHANGELOG";
@@ -49,16 +49,11 @@ DriveNotificationManager::DriveNotificationManager(
       push_notification_registered_(false),
       push_notification_enabled_(false),
       observers_notified_(false),
+      batch_timer_(clock),
       weak_ptr_factory_(this) {
   DCHECK(invalidation_service_);
   RegisterDriveNotifications();
   RestartPollingTimer();
-
-  batch_timer_ = std::make_unique<base::RetainingOneShotTimer>(
-      FROM_HERE, kInvalidationBatchInterval,
-      base::BindRepeating(&DriveNotificationManager::OnBatchTimerExpired,
-                          weak_ptr_factory_.GetWeakPtr()),
-      clock);
 }
 
 DriveNotificationManager::~DriveNotificationManager() {
@@ -122,12 +117,12 @@ void DriveNotificationManager::OnIncomingInvalidation(
   // See crbug.com/320878.
   invalidation_map.AcknowledgeAll();
 
-  if (!batch_timer_->IsRunning() && !invalidated_change_ids_.empty()) {
+  if (!batch_timer_.IsRunning() && !invalidated_change_ids_.empty()) {
     // Stop the polling timer as we'll be sending a batch soon.
     polling_timer_.Stop();
 
     // Restart the timer to send the batch when the timer fires.
-    batch_timer_->Reset();
+    RestartBatchTimer();
   }
 }
 
@@ -175,14 +170,30 @@ void DriveNotificationManager::RestartPollingTimer() {
   const int interval_secs = (push_notification_enabled_ ?
                              kSlowPollingIntervalInSecs :
                              kFastPollingIntervalInSecs);
+
+  int jitter = base::RandInt(0, interval_secs);
+
   polling_timer_.Stop();
   polling_timer_.Start(
-      FROM_HERE, base::TimeDelta::FromSeconds(interval_secs),
+      FROM_HERE, base::TimeDelta::FromSeconds(interval_secs + jitter),
       base::BindOnce(&DriveNotificationManager::NotifyObserversToUpdate,
                      weak_ptr_factory_.GetWeakPtr(), NOTIFICATION_POLLING,
                      std::map<std::string, int64_t>()));
 }
 
+void DriveNotificationManager::RestartBatchTimer() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  int jitter = base::RandInt(0, kInvalidationBatchIntervalSecs);
+
+  batch_timer_.Stop();
+  batch_timer_.Start(
+      FROM_HERE,
+      base::TimeDelta::FromSeconds(kInvalidationBatchIntervalSecs + jitter),
+      base::BindOnce(&DriveNotificationManager::OnBatchTimerExpired,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
 void DriveNotificationManager::NotifyObserversToUpdate(
     NotificationSource source,
     std::map<std::string, int64_t> invalidations) {

components/drive/drive_notification_manager.h

@@ -86,6 +86,10 @@ class DriveNotificationManager : public KeyedService,
   // Restarts the polling timer. Used for polling-based notification.
   void RestartPollingTimer();
 
+  // Restarts the batch notification timer. Used for batching together XMPP
+  // notifications so we can smooth out the traffic on the drive backends.
+  void RestartBatchTimer();
+
   // Notifies the observers that it's time to check for updates.
   // |source| indicates where the notification comes from.
   void NotifyObserversToUpdate(NotificationSource source,
@@ -123,7 +127,7 @@ class DriveNotificationManager : public KeyedService,
   // This timer is used to batch together invalidations. The invalidation
   // service can send many invalidations for the same id in rapid succession,
   // batching them together and removing duplicates is an optimzation.
-  std::unique_ptr<base::RetainingOneShotTimer> batch_timer_;
+  base::OneShotTimer batch_timer_;
 
   // The batch of invalidation id's that we've seen from the invaliation
   // service, will be reset when when send the invalidations to the observers.

components/drive/drive_notification_manager_unittest.cc

@@ -162,7 +162,7 @@ TEST_F(DriveNotificationManagerTest, TestBatchInvalidation) {
       syncer::Invalidation::InitUnknownVersion(kDefaultCorpusObjectId));
   EXPECT_TRUE(drive_notification_observer_->GetNotificationIds().empty());
 
-  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(5));
+  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(30));
 
   // Default corpus is has the id "" when sent to observers.
   std::map<std::string, int64_t> expected_ids = {{"", -1}};
@@ -181,7 +181,7 @@ TEST_F(DriveNotificationManagerTest, TestBatchInvalidation) {
       syncer::Invalidation::Init(kDefaultCorpusObjectId, 1, ""));
   EXPECT_TRUE(drive_notification_observer_->GetNotificationIds().empty());
 
-  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(5));
+  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(30));
 
   // Default corpus is has the id "" when sent to observers.
   expected_ids = {{"", 2}};
@@ -193,7 +193,7 @@ TEST_F(DriveNotificationManagerTest, TestBatchInvalidation) {
       syncer::Invalidation::Init(team_drive_1_object_id, 2, ""));
   EXPECT_TRUE(drive_notification_observer_->GetNotificationIds().empty());
 
-  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(5));
+  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(30));
   expected_ids = {{team_drive_id_1, 2}};
   EXPECT_EQ(expected_ids, drive_notification_observer_->GetNotificationIds());
   drive_notification_observer_->ClearNotificationIds();
@@ -214,7 +214,7 @@ TEST_F(DriveNotificationManagerTest, TestBatchInvalidation) {
 
   EXPECT_TRUE(drive_notification_observer_->GetNotificationIds().empty());
 
-  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(5));
+  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(30));
   expected_ids = {{"", 2}, {team_drive_id_1, 2}};
   EXPECT_EQ(expected_ids, drive_notification_observer_->GetNotificationIds());
 }