Parcourir la source

[2573] return the original double of progress, instead of convert to int.

so the caller can get highest possible precission.
JINMEI Tatuya il y a 12 ans
Parent
commit
fe931e2f1c

+ 9 - 7
src/lib/datasrc/tests/zone_loader_unittest.cc

@@ -323,9 +323,9 @@ TEST_F(ZoneLoaderTest, loadUnsigned) {
     EXPECT_EQ(34, destination_client_.rrsets_.size());
 
     // getRRCount should be identical of the RRs we've seen.  progress
-    // should reach 100%.
+    // should reach 100% (= 1).
     EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(100, loader.getProgress());
+    EXPECT_EQ(1, loader.getProgress());
 
     // Ensure known order.
     std::sort(destination_client_.rrsets_.begin(),
@@ -364,11 +364,13 @@ TEST_F(ZoneLoaderTest, loadUnsignedIncremental) {
     // 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.
+    // event.).  The expected value should be the exact expression that
+    // getProgress() should do internally, so EXPECT_EQ() should work here,
+    // but floating-point comparison can be always tricky we use
+    // EXPECT_DOUBLE_EQ just in case.
     EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(27, loader.getProgress()); // file size = 1541, offset = 428
+    // file size = 1541, offset = 428 (27.77%).
+    EXPECT_DOUBLE_EQ(static_cast<double>(428) / 1541, loader.getProgress());
 
     // We can finish the rest
     loader.loadIncremental(30);
@@ -377,7 +379,7 @@ TEST_F(ZoneLoaderTest, loadUnsignedIncremental) {
 
     // Counters are updated accordingly.  progress should reach 100%.
     EXPECT_EQ(destination_client_.rrsets_.size(), loader.getRRCount());
-    EXPECT_EQ(100, loader.getProgress());
+    EXPECT_EQ(1, loader.getProgress());
 
     // No more loading now
     EXPECT_THROW(loader.load(), isc::InvalidOperation);

+ 3 - 3
src/lib/datasrc/zone_loader.cc

@@ -35,7 +35,7 @@ using isc::dns::MasterLexer;
 namespace isc {
 namespace datasrc {
 
-const int ZoneLoader::PROGRESS_UNKNOWN = -1;
+const double ZoneLoader::PROGRESS_UNKNOWN = -1;
 
 ZoneLoader::ZoneLoader(DataSourceClient& destination, const Name& zone_name,
                        DataSourceClient& source) :
@@ -157,7 +157,7 @@ ZoneLoader::getRRCount() const {
     return (rr_count_);
 }
 
-int
+double
 ZoneLoader::getProgress() const {
     if (!loader_) {
         return (PROGRESS_UNKNOWN);
@@ -180,7 +180,7 @@ ZoneLoader::getProgress() const {
         return (PROGRESS_UNKNOWN);
     }
 
-    return ((static_cast<double>(pos) / total_size) * 100);
+    return (static_cast<double>(pos) / total_size);
 }
 
 } // end namespace datasrc

+ 11 - 11
src/lib/datasrc/zone_loader.h

@@ -150,23 +150,23 @@ public:
     /// \throw None
     size_t getRRCount() const;
 
-    /// \brief Return the current progress of the loader in percentage.
+    /// \brief Return the current progress of the loader.
     ///
-    /// 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.
+    /// This method returns the current estimated progress of loader as a
+    /// value between 0 and 1 (inclusive); it's 0 before starting the load,
+    /// and 1 at the completion, and a value between these (exclusive) 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.
+    /// 0 and 1.
     ///
     /// 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
+    /// to 1.  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).
@@ -187,13 +187,13 @@ public:
     /// source can provide the latter value efficiently.
     ///
     /// \throw None
-    int getProgress() const;
+    double getProgress() const;
 
     /// \brief A special value for \c getProgress, meaning the progress is
     /// unknown.
     ///
     /// See the method description for details.
-    static const int PROGRESS_UNKNOWN;
+    static const double PROGRESS_UNKNOWN;
 
 private:
     /// \brief The iterator used as source of data in case of the copy mode.

+ 1 - 1
src/lib/python/isc/datasrc/datasrc.cc

@@ -201,7 +201,7 @@ initModulePart_ZoneLoader(PyObject* mod) {
 
     try {
         installClassVariable(zone_loader_type, "PROGRESS_UNKNOWN",
-                             Py_BuildValue("i", ZoneLoader::PROGRESS_UNKNOWN));
+                             Py_BuildValue("d", ZoneLoader::PROGRESS_UNKNOWN));
     } catch (const std::exception& ex) {
         const std::string ex_what =
             "Unexpected failure in ZoneLoader initialization: " +

+ 10 - 4
src/lib/python/isc/datasrc/tests/zone_loader_test.py

@@ -117,9 +117,9 @@ class ZoneLoaderTests(unittest.TestCase):
         self.check_load()
 
         # Expected values are hardcoded, taken from the test zone file,
-        # assuming it won't change too often.  progress should reach 100%.
+        # assuming it won't change too often.  progress should reach 100% (=1).
         self.assertEqual(8, self.loader.get_rr_count())
-        self.assertEqual(100, self.loader.get_progress())
+        self.assertEqual(1, self.loader.get_progress())
 
     def test_load_from_client(self):
         self.source_client = isc.datasrc.DataSourceClient('sqlite3',
@@ -144,10 +144,16 @@ class ZoneLoaderTests(unittest.TestCase):
         self.check_zone_soa(ORIG_SOA_TXT)
 
         # In case it's from a zone file, get_progress should be in the middle
-        # of (0, 100).  expected value is taken from the test zone file
+        # of (0, 1).  expected value is taken from the test zone file
         # (total size = 422, current position = 288)
         if from_file:
-            self.assertEqual(int(288 * 100 / 422), self.loader.get_progress())
+            # To avoid any false positive due to rounding errors, we convert
+            # them to near integers between 0 and 100.
+            self.assertEqual(int((288 * 100) / 422),
+                             int(self.loader.get_progress() * 100))
+            # Also check the return value has higher precision.
+            self.assertNotEqual(int(288 * 100 / 422),
+                                100 * self.loader.get_progress())
 
         # After 5 more, it should return True (only having read 3)
         self.assertTrue(self.loader.load_incremental(5))

+ 12 - 10
src/lib/python/isc/datasrc/zone_loader_inc.cc

@@ -129,25 +129,27 @@ Exceptions:\n\
 \n\
 ";
 
+// Modifications
+// - double => float
 const char* const ZoneLoader_getProgress_doc = "\
-get_progress() -> int\n\
+get_progress() -> float\n\
 \n\
-Return the current progress of the loader in percentage.\n\
+Return the current progress of the loader.\n\
 \n\
-This method returns the current estimated progress of loader in\n\
-percentage; it's 0 before starting the load, and 100 at the\n\
-completion, and a value between 0 and 100 in the middle of loading.\n\
-It's an implementation detail how to calculate the progress, which may\n\
-vary depending on how the loader is constructed and may even be\n\
-impossible to detect effectively.\n\
+This method returns the current estimated progress of loader as a\n\
+value between 0 and 1 (inclusive); it's 0 before starting the load,\n\
+and 1 at the completion, and a value between these (exclusive) in the\n\
+middle of loading. It's an implementation detail how to calculate the\n\
+progress, which may vary depending on how the loader is constructed\n\
+and may even be impossible to detect effectively.\n\
 \n\
 If the progress cannot be determined, this method returns a special\n\
 value of PROGRESS_UNKNOWN, which is not included in the range between\n\
-0 and 100.\n\
+0 and 1.\n\
 \n\
 As such, the application should use the return value only for\n\
 informational purposes such as logging. For example, it shouldn't be\n\
-used to determine whether loading is completed by comparing it to 100.\n\
+used to determine whether loading is completed by comparing it to 1.\n\
 It should also expect the possibility of getting PROGRESS_UNKNOWN at\n\
 any call to this method; it shouldn't assume the specific way of\n\
 internal implementation as described below (which is provided for\n\

+ 1 - 1
src/lib/python/isc/datasrc/zone_loader_python.cc

@@ -196,7 +196,7 @@ ZoneLoader_getRRCount(PyObject* po_self, PyObject*) {
 PyObject*
 ZoneLoader_getProgress(PyObject* po_self, PyObject*) {
     s_ZoneLoader* self = static_cast<s_ZoneLoader*>(po_self);
-    return (Py_BuildValue("i", self->cppobj->getProgress()));
+    return (Py_BuildValue("d", self->cppobj->getProgress()));
 }
 
 // This list contains the actual set of functions we have in