[#78,!85] Addressed review comments.

diff --git a/doc/guide/hooks-ha.xml b/doc/guide/hooks-ha.xml
index 4b36ad598e257fba6215747411b716cfcd6ed3a3..6249cb5068f67de3c87719214708e09c0ec2161f 100644 (file)
--- a/doc/guide/hooks-ha.xml
+++ b/doc/guide/hooks-ha.xml
@@ -893,7 +893,7 @@
 
       <section xml:id="ha-sharing-lease-info">
         <title>Lease Information Sharing</title>
-        <para>The HA enabled server informs its active partner about allocated
+        <para>An HA enabled server informs its active partner about allocated
         or renewed leases by sending appropriate control commands. The partner
         updates the lease information in its own database. When the server starts
         up for the first time or recovers after a failure it synchronizes its
@@ -979,7 +979,7 @@
 
       <section xml:id="ha-syncing-page-limit">
         <title>Controlling Lease Page Size Limit</title>
-        <para>HA enabled server initiates synchronization of the lease
+        <para>An HA enabled server initiates synchronization of the lease
         database after down time or upon receiving <command>ha-sync</command>
         command. The server uses commands described in
         <xref linkend="lease-get-page-cmds"/> to fetch leases from the
@@ -1006,8 +1006,8 @@
         network, lease database synchronization after the server failure
         may be a time consuming operation. The synchronizing server needs
         to gather all leases from the partner which yields a large response
-        over the RESTful interface. The server receives leases using
-        paging mechanism as described in <xref linkend="ha-syncing-page-limit"/>.
+        over the RESTful interface. The server receives leases using the
+        paging mechanism described in <xref linkend="ha-syncing-page-limit"/>.
         Before the page of leases is fetched, the synchronizing server
         sends a <command>dhcp-disable</command> command to disable the DHCP
         service on the partner server. If the service is already disabled, this

diff --git a/src/hooks/dhcp/high_availability/ha_config.h b/src/hooks/dhcp/high_availability/ha_config.h
index ef48c8b671173dc075945a6853c11c5ec9b7f9b6..e11283444f556012965591a41f09be15b3856807 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_config.h
+++ b/src/hooks/dhcp/high_availability/ha_config.h
@@ -348,10 +348,10 @@ public:
         sync_timeout_ = sync_timeout;
     }
 
-    /// @brief Returns page size for leases fetched from the partner
+    /// @brief Returns maximum number of leases per page to be fetched
     /// during database synchronization.
     ///
-    /// @return Page limit.
+    /// @return Maximum number of leases per page.
     uint32_t getSyncPageLimit() const {
         return (sync_page_limit_);
     }

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index de9a3470e37c5a5fb1e036c56fd5453c365da4ae..470674eab025e8778811958aadb60977a42fc097 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -1022,10 +1022,10 @@ HAService::startHeartbeat() {
 }
 
 void
-HAService::asyncDisable(HttpClient& http_client,
-                        const std::string& server_name,
-                        const unsigned int max_period,
-                        PostRequestCallback post_request_action) {
+HAService::asyncDisableDHCPService(HttpClient& http_client,
+                                   const std::string& server_name,
+                                   const unsigned int max_period,
+                                   PostRequestCallback post_request_action) {
     HAConfig::PeerConfigPtr remote_config = config_->getPeerConfig(server_name);
 
     // Create HTTP/1.1 request including our command.
@@ -1091,9 +1091,9 @@ HAService::asyncDisable(HttpClient& http_client,
 }
 
 void
-HAService::asyncEnable(HttpClient& http_client,
-                       const std::string& server_name,
-                       PostRequestCallback post_request_action) {
+HAService::asyncEnableDHCPService(HttpClient& http_client,
+                                  const std::string& server_name,
+                                  PostRequestCallback post_request_action) {
     HAConfig::PeerConfigPtr remote_config = config_->getPeerConfig(server_name);
 
     // Create HTTP/1.1 request including our command.
@@ -1157,12 +1157,12 @@ HAService::asyncEnable(HttpClient& http_client,
 }
 
 void
-HAService::localDisable() {
+HAService::localDisableDHCPService() {
     network_state_->disableService();
 }
 
 void
-HAService::localEnable() {
+HAService::localEnableDHCPService() {
     network_state_->enableService();
 }
 
@@ -1174,7 +1174,8 @@ HAService::asyncSyncLeases() {
     unsigned int dhcp_disable_timeout =
         static_cast<unsigned int>(config_->getSyncTimeout() / 1000);
     if (dhcp_disable_timeout == 0) {
-        ++dhcp_disable_timeout;
+        // Ensure that we always use at least 1 second timeout.
+        dhcp_disable_timeout = 1;
     }
 
     asyncSyncLeases(client_, config_->getFailoverPeerConfig()->getName(),
@@ -1193,10 +1194,10 @@ HAService::asyncSyncLeases(http::HttpClient& http_client,
     // to allocate new leases while we fetch from it. The DHCP service will
     // be disabled for a certain amount of time and will be automatically
     // re-enabled if we die during the synchronization.
-    asyncDisable(http_client, server_name, max_period,
-                 [this, &http_client, server_name, max_period, last_lease,
-                  post_sync_action, dhcp_disabled]
-                 (const bool success, const std::string& error_message) {
+    asyncDisableDHCPService(http_client, server_name, max_period,
+                            [this, &http_client, server_name, max_period, last_lease,
+                             post_sync_action, dhcp_disabled]
+                            (const bool success, const std::string& error_message) {
 
         // If we have successfully disabled the DHCP service on the peer,
         // we can start fetching the leases.
@@ -1350,9 +1351,9 @@ HAService::asyncSyncLeasesInternal(http::HttpClient& http_client,
                                     (l + 1 == leases_element.end())) {
                                     last_lease = boost::dynamic_pointer_cast<Lease>(lease);
                                 }
-
                             }
 
+
                         } catch (const std::exception& ex) {
                             LOG_WARN(ha_logger, HA_LEASE_SYNC_FAILED)
                                 .arg((*l)->str())
@@ -1418,9 +1419,9 @@ HAService::synchronize(std::string& status_message, const std::string& server_na
         // we need to re-enable the DHCP service on the peer if the
         // DHCP service was disabled in the course of synchronization.
         if (dhcp_disabled) {
-            asyncEnable(client, server_name,
-                        [&](const bool success,
-                            const std::string& error_message) {
+            asyncEnableDHCPService(client, server_name,
+                                   [&](const bool success,
+                                       const std::string& error_message) {
                 // It is possible that we have already recorded an error
                 // message while synchronizing the lease database. Don't
                 // override the existing error message.

diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h
index bacef1fd99cc08fe0ebd09d563f55521bcb4139b..718b3314522ff4dc42fcee8b71f25a452aa8ab41 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.h
+++ b/src/hooks/dhcp/high_availability/ha_service.h
@@ -515,10 +515,10 @@ protected:
     /// should be disabled.
     /// @param post_request_action pointer to the function to be executed when
     /// the request is completed.
-    void asyncDisable(http::HttpClient& http_client,
-                      const std::string& server_name,
-                      const unsigned int max_period,
-                      PostRequestCallback post_request_action);
+    void asyncDisableDHCPService(http::HttpClient& http_client,
+                                 const std::string& server_name,
+                                 const unsigned int max_period,
+                                 PostRequestCallback post_request_action);
 
     /// @brief Schedules asynchronous "dhcp-enable" command to the specified
     /// server.
@@ -529,15 +529,15 @@ protected:
     /// sent.
     /// @param post_request_action pointer to the function to be executed when
     /// the request is completed.
-    void asyncEnable(http::HttpClient& http_client,
-                     const std::string& server_name,
-                     PostRequestCallback post_request_action);
+    void asyncEnableDHCPService(http::HttpClient& http_client,
+                                const std::string& server_name,
+                                PostRequestCallback post_request_action);
 
     /// @brief Disables local DHCP service.
-    void localDisable();
+    void localDisableDHCPService();
 
     /// @brief Enables local DHCP service.
-    void localEnable();
+    void localEnableDHCPService();
 
     /// @brief Asynchronously reads leases from a peer and updates local
     /// lease database.
@@ -574,10 +574,10 @@ protected:
     /// longer period of time. If the synchronization is progressing the
     /// timeout must be deferred.
     ///
-    /// The @c asyncSyncLeases method calls itself recursively when the
-    /// previous @c lease4-get-page or @c lease6-get-page command has
-    /// completed successfully. If the last page of leases was fetched or
-    /// if any error occurred, the synchronization is terminated and the
+    /// The @c asyncSyncLeases method calls itself (recurses) when the previous
+    /// @c lease4-get-page or @c lease6-get-page command has completed
+    /// successfully. If the last page of leases was fetched or if any
+    /// error occurred, the synchronization is terminated and the
     /// @c post_sync_action callback is invoked.
     ///
     /// The last parameter passed to the @c post_sync_action callback indicates

diff --git a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
index b57606c29e58d54d4825b8a43202b01afe505960..cedc2164b672791f014cf62cc1a712232579a3b4 100644 (file)
--- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
+++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
@@ -161,11 +161,11 @@ public:
     /// should be disabled.
     /// @param post_request_action pointer to the function to be executed when
     /// the request is completed.
-    void asyncDisable(const std::string& server_name,
-                      const unsigned int max_period,
-                      const PostRequestCallback& post_request_action) {
-        HAService::asyncDisable(client_, server_name, max_period,
-                                post_request_action);
+    void asyncDisableDHCPService(const std::string& server_name,
+                                 const unsigned int max_period,
+                                 const PostRequestCallback& post_request_action) {
+        HAService::asyncDisableDHCPService(client_, server_name, max_period,
+                                           post_request_action);
     }
 
     /// @brief Schedules asynchronous "dhcp-enable" command to the specified
@@ -177,9 +177,9 @@ public:
     /// sent.
     /// @param post_request_action pointer to the function to be executed when
     /// the request is completed.
-    void asyncEnable(const std::string& server_name,
-                     const PostRequestCallback& post_request_action) {
-        HAService::asyncEnable(client_, server_name, post_request_action);
+    void asyncEnableDHCPService(const std::string& server_name,
+                                const PostRequestCallback& post_request_action) {
+        HAService::asyncEnableDHCPService(client_, server_name, post_request_action);
     }
 
     using HAService::asyncSendHeartbeat;
@@ -2196,7 +2196,7 @@ TEST_F(HAServiceTest, processSynchronize6EnableError) {
 }
 
 // This test verifies that the DHCPv4 service can be disabled on the remote server.
-TEST_F(HAServiceTest, asyncDisable4) {
+TEST_F(HAServiceTest, asyncDisableDHCPService4) {
     // Create HA configuration.
     HAConfigPtr config_storage = createValidConfiguration();
 
@@ -2211,9 +2211,9 @@ TEST_F(HAServiceTest, asyncDisable4) {
 
     // Send dhcp-disable command with max-period of 10 seconds.
     // When the transaction is finished, the IO service gets stopped.
-    ASSERT_NO_THROW(service.asyncDisable("server3", 10,
-                                         [this](const bool success,
-                                                const std::string& error_message) {
+    ASSERT_NO_THROW(service.asyncDisableDHCPService("server3", 10,
+                                                    [this](const bool success,
+                                                           const std::string& error_message) {
         EXPECT_TRUE(success);
         EXPECT_TRUE(error_message.empty());
         io_service_->stop();
@@ -2231,7 +2231,7 @@ TEST_F(HAServiceTest, asyncDisable4) {
 
 // This test verifies that there is no exception thrown as a result of dhcp-disable
 // command when the server is offline.
-TEST_F(HAServiceTest, asyncDisable4ServerOffline) {
+TEST_F(HAServiceTest, asyncDisableDHCPService4ServerOffline) {
     // Create HA configuration.
     HAConfigPtr config_storage = createValidConfiguration();
 
@@ -2239,9 +2239,9 @@ TEST_F(HAServiceTest, asyncDisable4ServerOffline) {
 
     // Send dhcp-disable command with max-period of 10 seconds.
     // When the transaction is finished, the IO service gets stopped.
-    ASSERT_NO_THROW(service.asyncDisable("server2", 10,
-                                         [this](const bool success,
-                                                const std::string& error_message) {
+    ASSERT_NO_THROW(service.asyncDisableDHCPService("server2", 10,
+                                                    [this](const bool success,
+                                                           const std::string& error_message) {
         EXPECT_FALSE(success);
         EXPECT_FALSE(error_message.empty());
         io_service_->stop();
@@ -2253,7 +2253,7 @@ TEST_F(HAServiceTest, asyncDisable4ServerOffline) {
 
 // This test verifies that an error is returned when the remote server
 // returns control status error.
-TEST_F(HAServiceTest, asyncDisable4ControlResultError) {
+TEST_F(HAServiceTest, asyncDisableDHCPService4ControlResultError) {
     // Create HA configuration.
     HAConfigPtr config_storage = createValidConfiguration();
 
@@ -2273,9 +2273,9 @@ TEST_F(HAServiceTest, asyncDisable4ControlResultError) {
 
     // Send dhcp-disable command with max-period of 10 seconds.
     // When the transaction is finished, the IO service gets stopped.
-    ASSERT_NO_THROW(service.asyncDisable("server3", 10,
-                                         [this](const bool success,
-                                                const std::string& error_message) {
+    ASSERT_NO_THROW(service.asyncDisableDHCPService("server3", 10,
+                                                    [this](const bool success,
+                                                           const std::string& error_message) {
         EXPECT_FALSE(success);
         EXPECT_FALSE(error_message.empty());
         io_service_->stop();
@@ -2286,7 +2286,7 @@ TEST_F(HAServiceTest, asyncDisable4ControlResultError) {
 }
 
 // This test verifies that the DHCPv4 service can be enabled on the remote server.
-TEST_F(HAServiceTest, asyncEnable4) {
+TEST_F(HAServiceTest, asyncEnableDHCPService4) {
     // Create HA configuration.
     HAConfigPtr config_storage = createValidConfiguration();
 
@@ -2301,7 +2301,8 @@ TEST_F(HAServiceTest, asyncEnable4) {
 
     // Send dhcp-enable command. When the transaction is finished,
     // the IO service gets stopped.
-    ASSERT_NO_THROW(service.asyncEnable("server2", [this](const bool success,
+    ASSERT_NO_THROW(service.asyncEnableDHCPService("server2",
+                                                   [this](const bool success,
                                                           const std::string& error_message) {
         EXPECT_TRUE(success);
         EXPECT_TRUE(error_message.empty());
@@ -2319,7 +2320,7 @@ TEST_F(HAServiceTest, asyncEnable4) {
 
 // This test verifies that there is no exception thrown as a result of dhcp-enable
 // command when the server is offline.
-TEST_F(HAServiceTest, asyncEnable4ServerOffline) {
+TEST_F(HAServiceTest, asyncEnableDHCPService4ServerOffline) {
     // Create HA configuration.
     HAConfigPtr config_storage = createValidConfiguration();
 
@@ -2327,7 +2328,8 @@ TEST_F(HAServiceTest, asyncEnable4ServerOffline) {
 
     // Send dhcp-enable command. When the transaction is finished,
     // the IO service gets stopped.
-    ASSERT_NO_THROW(service.asyncEnable("server2", [this](const bool success,
+    ASSERT_NO_THROW(service.asyncEnableDHCPService("server2",
+                                                   [this](const bool success,
                                                           const std::string& error_message) {
         EXPECT_FALSE(success);
         EXPECT_FALSE(error_message.empty());
@@ -2340,7 +2342,7 @@ TEST_F(HAServiceTest, asyncEnable4ServerOffline) {
 
 // This test verifies that an error is returned when the remote server
 // returns control status error.
-TEST_F(HAServiceTest, asyncEnable4ControlResultError) {
+TEST_F(HAServiceTest, asyncEnableDHCPService4ControlResultError) {
     // Create HA configuration.
     HAConfigPtr config_storage = createValidConfiguration();
 
@@ -2360,7 +2362,8 @@ TEST_F(HAServiceTest, asyncEnable4ControlResultError) {
 
     // Send dhcp-enable command. When the transaction is finished,
     // the IO service gets stopped.
-    ASSERT_NO_THROW(service.asyncEnable("server2", [this](const bool success,
+    ASSERT_NO_THROW(service.asyncEnableDHCPService("server2",
+                                                   [this](const bool success,
                                                           const std::string& error_message) {
         EXPECT_FALSE(success);
         EXPECT_FALSE(error_message.empty());