fix: review comments

Author: zhjwpku

src/iceberg/manifest/rolling_manifest_writer.cc

@@ -32,9 +32,7 @@ RollingManifestWriter::RollingManifestWriter(
 
 RollingManifestWriter::~RollingManifestWriter() {
   // Ensure we close the current writer if not already closed
-  if (!closed_) {
-    (void)CloseCurrentWriter();
-  }
+  std::ignore = Close();
 }
 
 Status RollingManifestWriter::WriteAddedEntry(
@@ -96,13 +94,13 @@ bool RollingManifestWriter::ShouldRollToNewFile() const {
   if (current_writer_ == nullptr) {
     return false;
   }
-  // Roll when row count is a multiple of ROWS_DIVISOR and file size >= target
+  // Roll when row count is a multiple of the divisor and file size >= target
   if (current_file_rows_ % kRowsDivisor == 0) {
     auto length_result = current_writer_->length();
     if (length_result.has_value()) {
       return length_result.value() >= target_file_size_in_bytes_;
     }
-    // If we can't get the length, don't roll
+    // TODO(anyone): If we can't get the length, don't roll for now, revisit this later.
   }
   return false;
 }

src/iceberg/manifest/rolling_manifest_writer.h

@@ -35,9 +35,6 @@
 namespace iceberg {
 
 /// \brief A rolling manifest writer that can produce multiple manifest files.
-///
-/// As opposed to ManifestWriter, a rolling writer could produce multiple manifest
-/// files.
 class ICEBERG_EXPORT RollingManifestWriter {
  public:
   /// \brief Factory function type for creating ManifestWriter instances.
@@ -115,6 +112,8 @@ class ICEBERG_EXPORT RollingManifestWriter {
   /// \brief Close the current writer and add its ManifestFile to the list.
   Status CloseCurrentWriter();
 
+  /// \brief The number of rows after which to consider rolling to a new file.
+  /// \note This aligned with Iceberg's Java impl.
   static constexpr int64_t kRowsDivisor = 250;
 
   ManifestWriterFactory manifest_writer_factory_;