[2573] introduce getProgress instead size/position.

src/lib/datasrc/tests/zone_loader_unittest.cc

@@ -180,17 +180,16 @@ protected:
 };
 
 // Use the loader to load an unsigned zone.
-TEST_F(ZoneLoaderTest, copyUnsigned) {
+TEST_F(ZoneLoaderTest , copyUnsigned) {
     prepareSource(Name::ROOT_NAME(), "root.zone");
     ZoneLoader loader(destination_client_, Name::ROOT_NAME(), source_client_);
     // It gets the updater directly in the constructor
     ASSERT_EQ(1, destination_client_.provided_updaters_.size());
     EXPECT_EQ(Name::ROOT_NAME(), destination_client_.provided_updaters_[0]);
 
-    // Counters are initialized to 0.
+    // Counter is initialized to 0, progress is "unknown" in case of copy.
     EXPECT_EQ(0, loader.getRRCount());
-    EXPECT_EQ(0, loader.getSize());
-    EXPECT_EQ(0, loader.getPosition());
+    EXPECT_EQ(ZoneLoader::PROGRESS_UNKNOWN, loader.getProgress());
 
     // Now load the whole zone
     loader.load();
@@ -202,10 +201,9 @@ TEST_F(ZoneLoaderTest, copyUnsigned) {
     EXPECT_EQ(34, destination_client_.rrsets_.size());
 
     // Check various counters.  getRRCount should be identical of the RRs
-    // we've seen.  size and position should be still 0 in the copy operation.
+    // we've seen.  progress is still "unknown" in the copy operation.
     EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(0, loader.getSize());
-    EXPECT_EQ(0, loader.getPosition());
+    EXPECT_EQ(ZoneLoader::PROGRESS_UNKNOWN, loader.getProgress());
 
     // Ensure known order.
     std::sort(destination_client_.rrsets_.begin(),
@@ -234,11 +232,10 @@ TEST_F(ZoneLoaderTest, copyUnsignedIncremental) {
     // Not committed yet, we didn't complete the loading
     EXPECT_FALSE(destination_client_.commit_called_);
 
-    // Check we can get intermediate counters.  size/position are always 0
+    // Check we can get intermediate counters.  progress is always "unknown"
     // in case of copy.
     EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(0, loader.getSize());
-    EXPECT_EQ(0, loader.getPosition());
+    EXPECT_EQ(ZoneLoader::PROGRESS_UNKNOWN, loader.getProgress());
 
     // This is unusual, but allowed. Check it doesn't do anything
     loader.loadIncremental(0);
@@ -309,10 +306,9 @@ TEST_F(ZoneLoaderTest, loadUnsigned) {
     ZoneLoader loader(destination_client_, Name::ROOT_NAME(),
                       TEST_DATA_DIR "/root.zone");
 
-    // Counters are initialized to 0.
+    // Counter and progress are initialized to 0.
     EXPECT_EQ(0, loader.getRRCount());
-    EXPECT_EQ(0, loader.getSize());
-    EXPECT_EQ(0, loader.getPosition());
+    EXPECT_EQ(0, loader.getProgress());
 
     // It gets the updater directly in the constructor
     ASSERT_EQ(1, destination_client_.provided_updaters_.size());
@@ -326,12 +322,10 @@ TEST_F(ZoneLoaderTest, loadUnsigned) {
     // The count is 34 because we expect the RRs to be separated.
     EXPECT_EQ(34, destination_client_.rrsets_.size());
 
-    // Check various counters.  getRRCount should be identical of the RRs
-    // we've seen.  size and position should be identical of the RRs
-    // file (hardcoded, assuming we won't change it so often).
+    // getRRCount should be identical of the RRs we've seen.  progress
+    // should reach 100%.
     EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(1541, loader.getSize());
-    EXPECT_EQ(1541, loader.getPosition());
+    EXPECT_EQ(100, loader.getProgress());
 
     // Ensure known order.
     std::sort(destination_client_.rrsets_.begin(),
@@ -341,13 +335,6 @@ TEST_F(ZoneLoaderTest, loadUnsigned) {
     EXPECT_EQ("m.root-servers.net. 3600000 IN AAAA 2001:dc3::35\n",
               destination_client_.rrsets_.back());
 
-    // Check various counters.  getRRCount should be identical of the RRs
-    // we've seen.  size and position should be equal to the size of the zone
-    // file (hardcoded assuming we won't change it so often).
-    EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(1541, loader.getSize());
-    EXPECT_EQ(1541, loader.getPosition());
-
     // It isn't possible to try again now
     EXPECT_THROW(loader.load(), isc::InvalidOperation);
     EXPECT_THROW(loader.loadIncremental(1), isc::InvalidOperation);
@@ -362,8 +349,7 @@ TEST_F(ZoneLoaderTest, loadUnsignedIncremental) {
 
     // Counters are initialized to 0.
     EXPECT_EQ(0, loader.getRRCount());
-    EXPECT_EQ(0, loader.getSize());
-    EXPECT_EQ(0, loader.getPosition());
+    EXPECT_EQ(0, loader.getProgress());
 
     // Try loading few RRs first.
     loader.loadIncremental(10);
@@ -375,22 +361,23 @@ TEST_F(ZoneLoaderTest, loadUnsignedIncremental) {
     EXPECT_EQ(10, destination_client_.rrsets_.size());
     EXPECT_FALSE(destination_client_.commit_called_);
 
-    // Check we can get intermediate counters.  size is the size of the
-    // zone file; position is the offset to the end of 10th RR (both
-    // hardcoded).
+    // Check we can get intermediate counters.  expected progress is calculated
+    // based on the size of the zone file and the offset to the end of 10th RR
+    // (subject to future changes to the file, but we assume it's a rare
+    // event.).  This also involves floating point operation, which may
+    // cause a margin error depending on environments.  If it happens we
+    // should loosen the test condition to accept some margin.
     EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(1541, loader.getSize());
-    EXPECT_EQ(428, loader.getPosition());
+    EXPECT_EQ(27, loader.getProgress()); // file size = 1541, offset = 428
 
     // We can finish the rest
     loader.loadIncremental(30);
     EXPECT_EQ(34, destination_client_.rrsets_.size());
     EXPECT_TRUE(destination_client_.commit_called_);
 
-    // Counters are updated accordingly.  size and position are now identical.
+    // Counters are updated accordingly.  progress should reach 100%.
     EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(1541, loader.getSize());
-    EXPECT_EQ(1541, loader.getPosition());
+    EXPECT_EQ(100, loader.getProgress());
 
     // No more loading now
     EXPECT_THROW(loader.load(), isc::InvalidOperation);

src/lib/datasrc/zone_loader.cc

@@ -21,14 +21,20 @@
 #include <datasrc/zone.h>
 
 #include <dns/rrset.h>
+#include <dns/name.h>
+#include <dns/master_loader.h>
+#include <dns/master_lexer.h>
 
 using isc::dns::Name;
 using isc::dns::ConstRRsetPtr;
 using isc::dns::MasterLoader;
+using isc::dns::MasterLexer;
 
 namespace isc {
 namespace datasrc {
 
+const int ZoneLoader::PROGRESS_UNKNOWN = -1;
+
 ZoneLoader::ZoneLoader(DataSourceClient& destination, const Name& zone_name,
                        DataSourceClient& source) :
     // Separate the RRsets as that is possibly faster (the data source doesn't
@@ -134,20 +140,30 @@ ZoneLoader::getRRCount() const {
     return (rr_count_);
 }
 
-size_t
-ZoneLoader::getSize() const {
+int
+ZoneLoader::getProgress() const {
     if (!loader_) {
-        return (0);
+        return (PROGRESS_UNKNOWN);
     }
-    return (loader_->getSize());
-}
 
-size_t
-ZoneLoader::getPosition() const {
-    if (!loader_) {
+    const size_t pos = loader_->getPosition();
+    const size_t total_size = loader_->getSize();
+
+    // If the current position is 0, progress should definitely be 0; we
+    // don't bother to check the total size even if it's "unknown".
+    if (pos == 0) {
         return (0);
     }
-    return (loader_->getPosition());
+
+    // These cases shouldn't happen with our usage of MasterLoader.  So, in
+    // theory, we could throw here; however, since this method is expected
+    // to be used for informational purposes only, that's probably too harsh.
+    // So we return "unknown" instead.
+    if (total_size == MasterLexer::SOURCE_SIZE_UNKNOWN || total_size == 0) {
+        return (PROGRESS_UNKNOWN);
+    }
+
+    return ((static_cast<double>(pos) / total_size) * 100);
 }
 
 } // end namespace datasrc

src/lib/datasrc/zone_loader.h

@@ -150,60 +150,50 @@ public:
     /// \throw None
     size_t getRRCount() const;
 
-    /// \brief Return the (estimated) total size of the entire zone.
-    ///
-    /// This method returns some hint on how large the zone will be when
-    /// completing the load.  The returned size is a conceptual value that
-    /// can internally mean anything.  The intended usage of the value is
-    /// to compare it to the return value of \c getPosition() to estimate
-    /// the progress of the load at the time of the call.
+    /// \brief Return the current progress of the loader in percentage.
+    ///
+    /// This method returns the current estimated progress of loader in
+    /// percentage; it's 0 before starting the load, and 100 at the
+    /// completion, and a value between 0 and 100 in the middle of loading.
+    /// It's an implementation detail how to calculate the progress, which
+    /// may vary depending on how the loader is constructed and may even be
+    /// impossible to detect effectively.
+    ///
+    /// If the progress cannot be determined, this method returns a special
+    /// value of PROGRESS_UNKNOWN, which is not included in the range between
+    /// 0 and 100.
+    ///
+    /// As such, the application should use the return value only for
+    /// informational purposes such as logging.  For example, it shouldn't
+    /// be used to determine whether loading is completed by comparing it
+    /// to 100.  It should also expect the possibility of getting
+    /// \c PROGRESS_UNKNOWN at any call to this method; it shouldn't assume
+    /// the specific way of internal implementation as described below (which
+    /// is provided for informational purposes only).
     ///
     /// In this implementation, if the loader is constructed with a file
-    /// name, the returned size is the size of the zone file.  If it includes
-    /// other files via the $INCLUDE directive, it will be the sum of the
-    /// file sizes of all such files that the loader has handled.
-    /// Note that it may be smaller than the final size if there are more
-    /// files to be included which the loader has not seen by the time of
-    /// the call.
-    ///
-    /// Currently, if the loader is constructed with another data source
-    /// client, this method always returns 0.  In future, it may be possible
-    /// to return something more effective, e.g, the total number of RRs
-    /// if the underlying data source can provide that information efficiently.
-    ///
-    /// In any case, the caller shouldn't assume anything specific about the
-    /// meaning of the value other than for comparing it to the result of
-    /// \c getPosition().
+    /// name, the progress value is measured by the number of characters
+    /// read from the zone file divided by the size of the zone file
+    /// (with taking into account any included files).  Note that due to
+    /// the possibility of intermediate included files, the total file size
+    /// cannot be fully fixed until the completion of the load.  And, due to
+    /// this possibility, return values from this method may not always
+    /// increase monotonically.
+    ///
+    /// If it's constructed with another data source client, this method
+    /// always returns \c PROGRESS_UNKNOWN; in future, however, it may become
+    /// possible to return something more useful, e.g, based on the result
+    /// of \c getRRCount() and the total number of RRs if the underlying data
+    /// source can provide the latter value efficiently.
     ///
     /// \throw None
-    size_t getSize() const;
+    int getProgress() const;
 
-    /// \brief Return the current position of the loader in the zone being
-    /// loaded.
-    ///
-    /// This method returns a conceptual "position" of this loader in the
-    /// loader relative to the return value of \c getSize().  Before starting
-    /// the load the position is set to 0; on successful completion,
-    /// it will be equal to the \c getSize() value; in the middle of the load,
-    /// it's expected to be between these values, which would give some
-    /// hint about the progress of the loader.
-    ///
-    /// In the current implementation, if the loader is constructed with a
-    /// file name, the returned value is the number of characters from the
-    /// zone file (and any included files) recognized by the underlying zone
-    /// file parser.
+    /// \brief A special value for \c getProgress, meaning the progress is
+    /// unknown.
     ///
-    /// If it's constructed with another data source client, it's always
-    /// 0 for now; however, if \c getPosition() is extended in this case
-    /// as documented (see the method description), the result of
-    /// \c getRRCount() could be used for the current position.
-    ///
-    /// Like \c getSize(), the value is conceptual and the caller shouldn't
-    /// assume any specific meaning of the value except for comparing it
-    /// to \c getSize() results.
-    ///
-    /// \throw None
-    size_t getPosition() const;
+    /// See the method description for details.
+    static const int PROGRESS_UNKNOWN;
 
 private:
     /// \brief The iterator used as source of data in case of the copy mode.