Commit f004912d authored by Thomas Markwalder's avatar Thomas Markwalder
Browse files

[3075] Address review comments.

Minor corrections only, there were no major revisions.
parent c39eb9bb
/Users/tmark/ddns/build/new3075/bind10
# Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
#
# Permission to use, copy, modify, and/or distribute this software for any
......@@ -173,7 +172,7 @@ needs to be increased, the DHCP-DDNS clients are simply generating too many
requests too quickly, or perhaps upstream DNS servers are experiencing
load issues.
% DHCP_DDNS_QUEUE_MGR_RECONFIG application is reconfiguring the queue manager
% DHCP_DDNS_QUEUE_MGR_RECONFIGURING application is reconfiguring the queue manager
This is an informational message indicating that DHCP_DDNS is reconfiguring the
queue manager as part of normal startup or in response to a new configuration.
......@@ -199,7 +198,7 @@ manager if given a new configuration.
% DHCP_DDNS_QUEUE_MGR_RESUMING application is resuming listening for requests now that the request queue size has reached %1 of a maximum %2 allowed
This is an informational message indicating that DHCP_DDNS, which had stopped
accpeting new requests, has processed enough entries from the receive queue to
accepting new requests, has processed enough entries from the receive queue to
resume accepting requests.
% DHCP_DDNS_QUEUE_MGR_STARTED application's queue manager has begun listening for requests.
......@@ -229,6 +228,12 @@ trying to stop the queue manager. This error is unlikely to occur or to
impair the application's ability to function but it should be reported for
analysis.
% DHCP_DDNS_QUEUE_MGR_UNEXPECTED_HANDLER_ERROR application's queue manager request receive handler experienced an unexpected exception %1:
This is an error message indicating that an unexpected error occurred within the
DHCP_DDNS's Queue Manager request receive completion handler. This is most
likely a programmatic issue that should be reported. The application may
recover on its own.
% DHCP_DDNS_QUEUE_MGR_UNEXPECTED_STOP application's queue manager receive was
aborted unexpectedly while queue manager state is: %1
This is an error message indicating that DHCP_DDNS's Queue Manager request
......
......@@ -30,7 +30,7 @@ const char* D2Process::SD_INVALID_STR = "invalid";
// Setting to 80% for now. This is an arbitrary choice and should probably
// be configurable.
const float D2Process::QUEUE_RESTART_PERCENT = 0.80;
const unsigned int D2Process::QUEUE_RESTART_PERCENT = 80;
D2Process::D2Process(const char* name, IOServicePtr io_service)
: DProcessBase(name, io_service, DCfgMgrBasePtr(new D2CfgMgr())),
......@@ -120,11 +120,11 @@ D2Process::runIO() {
cnt = asio_io_service.run_one();
}
return cnt;
return (cnt);
}
bool
D2Process::canShutdown() {
D2Process::canShutdown() const {
bool all_clear = false;
// If we have been told to shutdown, find out if we are ready to do so.
......@@ -151,6 +151,11 @@ D2Process::canShutdown() {
// Get out right now, no niceties.
all_clear = true;
break;
default:
// shutdown_type_ is an enum and should only be one of the above.
// if its getting through to this, something is whacked.
break;
}
if (all_clear) {
......@@ -260,8 +265,8 @@ D2Process::checkQueueStatus() {
// Resume receiving once the queue has decreased by twenty
// percent. This is an arbitrary choice. @todo this value should
// probably be configurable.
size_t threshold = (queue_mgr_->getMaxQueueSize()
* QUEUE_RESTART_PERCENT);
size_t threshold = (((queue_mgr_->getMaxQueueSize()
* QUEUE_RESTART_PERCENT)) / 100);
if (queue_mgr_->getQueueSize() <= threshold) {
LOG_INFO (dctl_logger, DHCP_DDNS_QUEUE_MGR_RESUMING)
.arg(threshold).arg(queue_mgr_->getMaxQueueSize());
......@@ -302,7 +307,7 @@ D2Process::checkQueueStatus() {
// we can do the reconfigure. In other words, we aren't RUNNING or
// STOPPING.
if (reconf_queue_flag_) {
LOG_INFO (dctl_logger, DHCP_DDNS_QUEUE_MGR_RECONFIG);
LOG_INFO (dctl_logger, DHCP_DDNS_QUEUE_MGR_RECONFIGURING);
reconfigureQueueMgr();
}
break;
......@@ -372,7 +377,7 @@ D2Process::getD2CfgMgr() {
return (boost::dynamic_pointer_cast<D2CfgMgr>(getCfgMgr()));
}
const char* D2Process::getShutdownTypeStr(ShutdownType type) {
const char* D2Process::getShutdownTypeStr(const ShutdownType& type) {
const char* str = SD_INVALID_STR;
switch (type) {
case SD_NORMAL:
......@@ -384,6 +389,8 @@ const char* D2Process::getShutdownTypeStr(ShutdownType type) {
case SD_NOW:
str = SD_NOW_STR;
break;
default:
break;
}
return (str);
......
......@@ -33,6 +33,14 @@ class D2Process : public DProcessBase {
public:
/// @brief Defines the shutdown types supported by D2Process
///
/// * SD_NORMAL - Stops the queue manager and finishes all current
/// transactions before exiting. This is the default.
///
/// * SD_DRAIN_FIRST - Stops the queue manager but continues processing
/// requests from the queue until it is empty.
///
/// * SD_NOW - Exits immediately.
enum ShutdownType {
SD_NORMAL,
SD_DRAIN_FIRST,
......@@ -53,11 +61,11 @@ public:
/// state. Once the number of entries has decreased to this percentage
/// of the maximum allowed, D2Process will "resume" receiving requests
/// by restarting the queue manager.
static const float QUEUE_RESTART_PERCENT;
static const unsigned int QUEUE_RESTART_PERCENT;
/// @brief Constructor
///
/// The construction process creates the configuration manager, the queue
/// Construction creates the configuration manager, the queue
/// manager, and the update manager.
///
/// @param name name is a text label for the process. Generally used
......@@ -71,11 +79,12 @@ public:
/// @brief Called after instantiation to perform initialization unique to
/// D2.
///
/// This is called after command line arguments but PRIOR to configuration
/// reception. The base class provides this method as a place to perform
/// any derivation-specific initialization steps that are inapppropriate
/// for the constructor but necessary prior to launch. So far, no such
/// steps have been identified for D2, so its implementantion is empty.
/// This is invoked by the controller after command line arguments but
/// PRIOR to configuration reception. The base class provides this method
/// as a place to perform any derivation-specific initialization steps
/// that are inapppropriate for the constructor but necessary prior to
/// launch. So far, no such steps have been identified for D2, so its
/// implementantion is empty but required.
///
/// @throw DProcessBaseError if the initialization fails.
virtual void init();
......@@ -84,7 +93,7 @@ public:
///
/// Once entered, the main control thread remains inside this method
/// until shutdown. The event loop logic is as follows:
/// {{{
/// @code
/// while should not down {
/// process queue manager state change
/// process completed jobs
......@@ -93,7 +102,7 @@ public:
///
/// ON an exception, exit with fatal error
/// }
/// }}}
/// @endcode
///
/// To summarize, each pass through the event loop first checks the state
/// of the received queue and takes any steps required to ensure it is
......@@ -233,7 +242,7 @@ protected:
/// If callbacks are ready to be executed upon entry, the method will
/// return as soon as these callbacks have completed. If no callbacks
/// are ready, then it will wait (indefinitely) until at least one callback
/// is executed. (NOTE: Should become desirable to periodically force an
/// is executed. (@note: Should become desirable to periodically force an
/// event, an interval timer could be used to do so).
///
/// @return The number of callback handlers executed, or 0 if the IO
......@@ -251,14 +260,14 @@ protected:
/// met.
///
/// @return Returns true if the criteria has been met, false otherwise.
virtual bool canShutdown();
virtual bool canShutdown() const;
/// @brief Sets queue reconfigure indicator to the given value.
///
/// @param value is the new value to assign to the indicator
///
/// NOTE this method is really only intended for testing purposes.
void setReconfQueueFlag(bool value) {
/// @note this method is really only intended for testing purposes.
void setReconfQueueFlag(const bool value) {
reconf_queue_flag_ = value;
}
......@@ -266,8 +275,8 @@ protected:
///
/// @param value is the new value to assign to shutdown type.
///
/// NOTE this method is really only intended for testing purposes.
void setShutdownType(ShutdownType value) {
/// @note this method is really only intended for testing purposes.
void setShutdownType(const ShutdownType& value) {
shutdown_type_ = value;
}
......@@ -278,12 +287,12 @@ public:
D2CfgMgrPtr getD2CfgMgr();
/// @brief Returns a reference to the queue manager.
D2QueueMgrPtr& getD2QueueMgr() {
const D2QueueMgrPtr& getD2QueueMgr() const {
return (queue_mgr_);
}
/// @brief Returns a reference to the update manager.
D2UpdateMgrPtr& getD2UpdateMgr() {
const D2UpdateMgrPtr& getD2UpdateMgr() const {
return (update_mgr_);
}
......@@ -305,7 +314,7 @@ public:
///
/// @return A text label corresponding the value or "invalid" if the
/// value is not a valid value.
static const char* getShutdownTypeStr(ShutdownType type);
static const char* getShutdownTypeStr(const ShutdownType& type);
private:
/// @brief Pointer to our queue manager instance.
......
......@@ -36,54 +36,60 @@ D2QueueMgr::~D2QueueMgr() {
void
D2QueueMgr::operator()(const dhcp_ddns::NameChangeListener::Result result,
dhcp_ddns::NameChangeRequestPtr& ncr) {
// Note that error conditions must be handled here without throwing
// exceptions. Remember this is the application level "link" in the
// callback chain. Throwing an exception here will "break" the
// io_service "run" we are operating under. With that in mind,
// if we hit a problem, we will stop the listener transition to
// the appropriate stopped state. Upper layer(s) must monitor our
// state as well as our queue size.
switch (result) {
case dhcp_ddns::NameChangeListener::SUCCESS:
// Receive was successful, attempt to queue the request.
if (getQueueSize() < getMaxQueueSize()) {
// There's room on the queue, add to the end
enqueue(ncr);
return;
}
// Queue is full, stop the listener.
// Note that we can move straight to a STOPPED state as there
// is no receive in progress.
LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_QUEUE_FULL)
.arg(max_queue_size_);
stopListening(STOPPED_QUEUE_FULL);
break;
case dhcp_ddns::NameChangeListener::STOPPED:
if (mgr_state_ == STOPPING) {
// This is confirmation that the listener has stopped and its
// callback will not be called again, unless its restarted.
updateStopState();
} else {
// We should not get an receive complete status of stopped unless
// we canceled the read as part of stopping. Therefore this is
// unexpected so we will treat it as a receive error.
// This is most likely an unforeseen programmatic issue.
LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_UNEXPECTED_STOP)
.arg(mgr_state_);
try {
// Note that error conditions must be handled here without throwing
// exceptions. Remember this is the application level "link" in the
// callback chain. Throwing an exception here will "break" the
// io_service "run" we are operating under. With that in mind,
// if we hit a problem, we will stop the listener transition to
// the appropriate stopped state. Upper layer(s) must monitor our
// state as well as our queue size.
switch (result) {
case dhcp_ddns::NameChangeListener::SUCCESS:
// Receive was successful, attempt to queue the request.
if (getQueueSize() < getMaxQueueSize()) {
// There's room on the queue, add to the end
enqueue(ncr);
return;
}
// Queue is full, stop the listener.
// Note that we can move straight to a STOPPED state as there
// is no receive in progress.
LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_QUEUE_FULL)
.arg(max_queue_size_);
stopListening(STOPPED_QUEUE_FULL);
break;
case dhcp_ddns::NameChangeListener::STOPPED:
if (mgr_state_ == STOPPING) {
// This is confirmation that the listener has stopped and its
// callback will not be called again, unless its restarted.
updateStopState();
} else {
// We should not get an receive complete status of stopped
// unless we canceled the read as part of stopping. Therefore
// this is unexpected so we will treat it as a receive error.
// This is most likely an unforeseen programmatic issue.
LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_UNEXPECTED_STOP)
.arg(mgr_state_);
stopListening(STOPPED_RECV_ERROR);
}
break;
default:
// Receive failed, stop the listener.
// Note that we can move straight to a STOPPED state as there
// is no receive in progress.
LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_RECV_ERROR);
stopListening(STOPPED_RECV_ERROR);
break;
}
break;
default:
// Receive failed, stop the listener.
// Note that we can move straight to a STOPPED state as there
// is no receive in progress.
LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_RECV_ERROR);
stopListening(STOPPED_RECV_ERROR);
break;
} catch (const std::exception& ex) {
// On the outside chance a throw occurs, let's log it and swallow it.
LOG_ERROR(dctl_logger, DHCP_DDNS_QUEUE_MGR_UNEXPECTED_HANDLER_ERROR)
.arg(ex.what());
}
}
......
......@@ -35,12 +35,17 @@ public:
isc::Exception(file, line, what) { };
};
/// @brief String value for the shutdown command.
static const std::string SHUT_DOWN_COMMAND("shutdown");
/// @brief Returned by the process to indicate a command was successful.
static const int COMMAND_SUCCESS = 0;
/// @brief Returned by the process to indicates a command failed.
static const int COMMAND_ERROR = 1;
static const int COMMAND_INVALID = 2;
static const std::string SHUT_DOWN_COMMAND("shutdown");
static const int CONFIG_INVALID = 1;
/// @brief Returned by the process to indicates a command is not valid.
static const int COMMAND_INVALID = 2;
/// @brief Application Process Interface
///
......@@ -134,7 +139,12 @@ public:
/// @param args is a set of arguments (if any) required for the given
/// command.
/// @return an Element that contains the results of command composed
/// of an integer status value (0 means successful, non-zero means failure),
/// of an integer status value:
///
/// - COMMAND_SUCCESS indicates a command was successful.
/// - COMMAND_ERROR indicates a valid command failed execute.
/// - COMMAND_INVALID indicates a command is not valid.
///
/// and a string explanation of the outcome.
virtual isc::data::ConstElementPtr command(
const std::string& command, isc::data::ConstElementPtr args) = 0;
......@@ -145,7 +155,7 @@ public:
/// @brief Checks if the process has been instructed to shut down.
///
/// @return true if process shutdown flag is true.
bool shouldShutdown() {
bool shouldShutdown() const {
return (shut_down_flag_);
}
......@@ -158,7 +168,7 @@ public:
/// @brief Fetches the application name.
///
/// @return a the application name string.
/// @return application name string.
const std::string getAppName() const {
return (app_name_);
}
......
......@@ -195,7 +195,7 @@
"commands": [
{
"command_name": "shutdown",
"command_description": "Shuts down DHCP_DDNS server.",
"command_description": "Shuts down b1-dhcp-ddns module server.",
"command_args": [
{
"item_name": "type",
......
......@@ -347,8 +347,7 @@ TEST_F(TSIGKeyInfoTest, invalidTSIGKeyList) {
ASSERT_NO_THROW(parser.reset(new TSIGKeyInfoListParser("test", keys_)));
// Verify that the list builds without errors.
//ASSERT_NO_THROW(parser->build(config_set_));
parser->build(config_set_);
ASSERT_NO_THROW(parser->build(config_set_));
// Verify that the list commit fails.
EXPECT_THROW(parser->commit(), D2CfgError);
......
......@@ -113,7 +113,7 @@ public:
// Must call checkQueueStatus, to cause queue manager to reconfigure
// and start.
checkQueueStatus();
D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
const D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
// If queue manager isn't in the RUNNING state, return failure.
if (D2QueueMgr::RUNNING != queue_mgr->getMgrState()) {
......@@ -161,7 +161,7 @@ TEST(D2Process, construction) {
D2QueueMgrPtr queue_mgr = d2process.getD2QueueMgr();
ASSERT_TRUE(queue_mgr);
D2UpdateMgrPtr& update_mgr = d2process.getD2UpdateMgr();
const D2UpdateMgrPtr& update_mgr = d2process.getD2UpdateMgr();
ASSERT_TRUE(update_mgr);
}
......@@ -219,7 +219,7 @@ TEST_F(D2ProcessTest, configure) {
/// stop is initiated.
TEST_F(D2ProcessTest, queueStopOnShutdown) {
ASSERT_TRUE(runWithConfig(valid_d2_config));
D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
const D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
setShutdownFlag(true);
......@@ -247,7 +247,7 @@ TEST_F(D2ProcessTest, queueStopOnShutdown) {
/// manager stop is initiated.
TEST_F(D2ProcessTest, queueStopOnReconf) {
ASSERT_TRUE(runWithConfig(valid_d2_config));
D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
const D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
// Manually set the reconfigure indicator.
setReconfQueueFlag(true);
......@@ -285,7 +285,7 @@ TEST_F(D2ProcessTest, queueFullRecovery) {
// Start queue manager with known good config.
ASSERT_TRUE(runWithConfig(valid_d2_config));
D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
const D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
// Set the maximum queue size to manageable number.
size_t max_queue_size = 5;
......@@ -335,7 +335,7 @@ TEST_F(D2ProcessTest, queueFullRecovery) {
/// verifies that checkQueueStatus reacts properly to recover.
TEST_F(D2ProcessTest, queueErrorRecovery) {
ASSERT_TRUE(runWithConfig(valid_d2_config));
D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
const D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
// Since we are not really receiving, we have to stage an error.
queue_mgr->stopListening(D2QueueMgr::STOPPED_RECV_ERROR);
......@@ -474,7 +474,7 @@ TEST_F(D2ProcessTest, shutdownArgs) {
/// returning its result.
TEST_F(D2ProcessTest, canShutdown) {
ASSERT_TRUE(runWithConfig(valid_d2_config));
D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
const D2QueueMgrPtr& queue_mgr = getD2QueueMgr();
// Shutdown flag is false. Method should return false for all types.
EXPECT_EQ(false, checkCanShutdown(SD_NORMAL));
......@@ -539,7 +539,7 @@ TEST_F(D2ProcessTest, canShutdown) {
// Now use update manager to dequeue the request and make a transaction.
// This lets us verify transaction list not empty logic.
D2UpdateMgrPtr& update_mgr = getD2UpdateMgr();
const D2UpdateMgrPtr& update_mgr = getD2UpdateMgr();
ASSERT_TRUE(update_mgr);
ASSERT_NO_THROW(update_mgr->sweep());
ASSERT_EQ(0, queue_mgr->getQueueSize());
......
......@@ -285,7 +285,7 @@ public:
return (listening_);
}
/// @brief Returns true if the listener is has an IO call in progress.
/// @brief Returns true if the listener has an IO call in progress.
///
/// A true value indicates that the listener has an asynchronous IO in
/// progress which will complete at some point in the future. Completion
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment