Skip to content

fix(cpp): flush pending data on writer close - #969

Merged
ColinLeeo merged 1 commit into
apache:developfrom
ColinLeeo:colin/close-flush-on-writer-close
Sep 24, 2026
Merged

ColinLeeo merged 1 commit into
apache:developfrom
ColinLeeo:colin/close-flush-on-writer-close

Conversation

@ColinLeeo

Copy link
Copy Markdown
Contributor

Summary

  • Flush pending buffered data before finalizing a C++ TsFile writer.
  • Propagate flush errors without writing the index/footer.
  • Remove redundant flush calls from C ABI close wrappers and add regression coverage.

Testing

  • cmake --build cpp/target/build --target TsFile_Test -j2
  • Relevant writer/C-wrapper tests: 55 passed; final focused rerun: 12 passed.

@ColinLeeo
ColinLeeo requested review from hongzhi-gao and jt2594838 and a lite review from Copilot and removed request for hongzhi-gao and jt2594838 September 23, 2026 09:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 High severity · 3 Medium severity

Open (4)
What changed in this PR

Flushes buffered data as part of writer close to avoid missing trailing writes, and simplifies C ABI close wrappers by relying on the writer’s close semantics.

Changes:

  • Call flush() inside TsFileWriter::close() and propagate flush failures before finalization.
  • Remove redundant explicit flush() calls from C wrapper close functions.
  • Add/extend regression tests verifying close persists data written after the last flush (writer + table view).
File Description
cpp/​test/​writer/​tsfile_writer_test.cc Adds a regression test ensuring close() flushes data written after the last flush().
cpp/​test/​writer/​table_view/​tsfile_writer_table_test.cc Updates table writer test to verify data is readable after close() (without explicit flush()).
cpp/​src/​writer/​tsfile_writer.h Updates close() documentation to reflect the new flush-and-finalize behavior.
cpp/​src/​writer/​tsfile_writer.cc Implements close() to flush() before calling end_file().
cpp/​src/​writer/​tsfile_table_writer.h Updates close() documentation to mention flushing/finalization.
cpp/​src/​cwrapper/​tsfile_cwrapper.h Updates C API docstring to mention flush/finalize behavior on close.
cpp/​src/​cwrapper/​tsfile_cwrapper.cc Removes explicit flush() from C ABI close wrappers and relies on close().

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

++row_count;
}
EXPECT_EQ(row_count, 10);
table_result_set->close();
Comment on lines +499 to +500
* @brief Flushes pending data, finalizes the TsFile, and releases resources
* associated with a TsFileWriter.
Comment on lines +1989 to 1993
int ret = flush();
if (ret != E_OK) {
return ret;
}
return io_writer_->end_file();
std::vector<std::string> select_list = {device_path + "." +
measurement_name};
ASSERT_EQ(reader.query(select_list, 100, 102, result_set), E_OK);
auto* query_result = static_cast<QDSWithoutTimeGenerator*>(result_set);
@ColinLeeo
ColinLeeo merged commit 83fe559 into apache:develop Sep 24, 2026
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants